From 17e4c4c948558a12151572701fa76c586852a858 Mon Sep 17 00:00:00 2001 From: Damien Pellier Date: Mon, 21 Oct 2024 10:03:08 +0200 Subject: [PATCH] fix(modal): prevent odsClose event on unmount if closed --- .../src/components/ods-modal/ods-modal.tsx | 8 +- .../ods/src/components/modal/src/index.html | 158 ++++++++++-------- .../modal/tests/behaviour/ods-modal.e2e.ts | 98 +++++++---- .../components/modal/documentation.mdx | 2 +- 4 files changed, 159 insertions(+), 107 deletions(-) diff --git a/packages/ods/src/components/modal/src/components/ods-modal/ods-modal.tsx b/packages/ods/src/components/modal/src/components/ods-modal/ods-modal.tsx index 2c93a9dcd5..c48f1c9562 100644 --- a/packages/ods/src/components/modal/src/components/ods-modal/ods-modal.tsx +++ b/packages/ods/src/components/modal/src/components/ods-modal/ods-modal.tsx @@ -84,8 +84,14 @@ export class OdsModal { disconnectedCallback(): void { document.body.style.removeProperty('overflow'); this.modalDialog?.close?.(); + + // TODO we should not emit on disconnectedCallback as the emitter does not exists anymore + // this should be refactored with some use-case example on FAQ, especially regarding react-testing-library + // (do this only when we have working react-testing-library on our side to validate every use-case) + if (this.isOpen) { + this.odsClose.emit(); + } this.isOpen = false; - this.odsClose.emit(); } handleBackdropClick(event: MouseEvent): void { diff --git a/packages/ods/src/components/modal/src/index.html b/packages/ods/src/components/modal/src/index.html index 0584adf2b1..f95260e333 100644 --- a/packages/ods/src/components/modal/src/index.html +++ b/packages/ods/src/components/modal/src/index.html @@ -8,80 +8,6 @@ - - - - - - - - - - - Hello, world! - - - Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. - - - - - - - - Hello, world! - - - Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. - - - - - - Hello, world! - - - Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. - - - - - - - - Hello, world! - - - Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Modal test + + diff --git a/packages/ods/src/components/modal/tests/behaviour/ods-modal.e2e.ts b/packages/ods/src/components/modal/tests/behaviour/ods-modal.e2e.ts index 14d6a3fd26..e64f27c996 100644 --- a/packages/ods/src/components/modal/tests/behaviour/ods-modal.e2e.ts +++ b/packages/ods/src/components/modal/tests/behaviour/ods-modal.e2e.ts @@ -27,51 +27,87 @@ describe('ods-modal behaviour', () => { }); } - it('should trigger open event when opening', async() => { - await setup('Hello, world!'); - const openSpy = await page.spyOnEvent('odsOpen'); - const modal = await page.find('ods-modal'); + describe('events', () => { + describe('odsClose', () => { + it('should trigger when closing', async() => { + await setup('Hello, world!'); + const closeSpy = await page.spyOnEvent('odsClose'); + const modal = await page.find('ods-modal'); - await modal.callMethod('open'); + await modal.callMethod('close'); + await waitForAnimationEnd(); - expect(openSpy).toHaveReceivedEventTimes(1); - }); + expect(closeSpy).toHaveReceivedEventTimes(1); + }); - it('should trigger close event when closing', async() => { - await setup('Hello, world!'); - const closeSpy = await page.spyOnEvent('odsClose'); - const modal = await page.find('ods-modal'); + it('should trigger when unmounting if opened', async() => { + await setup('Hello, world!'); + const modalHandle = await page.waitForSelector('ods-modal'); + const modal = await page.find('ods-modal'); + const closeSpy = await modal.spyOnEvent('odsClose'); - await modal.callMethod('close'); - await waitForAnimationEnd(); + await modalHandle?.evaluate((element) => element.remove()); + await page.waitForChanges(); - expect(closeSpy).toHaveReceivedEventTimes(1); - }); + expect(closeSpy).toHaveReceivedEventTimes(1); + }); - it('should open on element open method call', async() => { - await setup('Hello, world!'); - const modal = await page.find('ods-modal'); + it('should not trigger when unmounting if closed', async() => { + await setup('Hello, world!'); + const modalHandle = await page.waitForSelector('ods-modal'); + const modal = await page.find('ods-modal'); + const closeSpy = await modal.spyOnEvent('odsClose'); - await modal.callMethod('open'); - await page.waitForChanges(); + await modalHandle?.evaluate((element) => element.remove()); + await page.waitForChanges(); - const isOpen = await page.evaluate(() => { - const dialog = document.querySelector('ods-modal')?.shadowRoot?.querySelector('.ods-modal__dialog') as HTMLDialogElement; - return dialog && dialog.hasAttribute('open'); + expect(closeSpy).not.toHaveReceivedEvent(); + }); }); - expect(isOpen).toBe(true); + describe('odsOpen', () => { + it('should trigger when opening', async() => { + await setup('Hello, world!'); + const openSpy = await page.spyOnEvent('odsOpen'); + const modal = await page.find('ods-modal'); + + await modal.callMethod('open'); + + expect(openSpy).toHaveReceivedEventTimes(1); + }); + }); }); - it('should add a close animation on element close method call', async() => { - await setup('Hello, world!'); - const modal = await page.find('ods-modal'); + describe('methods', () => { + describe('close', () => { + it('should add a close animation on element close method call', async() => { + await setup('Hello, world!'); + const modal = await page.find('ods-modal'); - await modal.callMethod('close'); - await page.waitForChanges(); + await modal.callMethod('close'); + await page.waitForChanges(); + + const closeAnimation = await page.find('ods-modal >>> .ods-modal__dialog--close-animation'); + + expect(closeAnimation).not.toBeNull(); + }); + }); - const closeAnimation = await page.find('ods-modal >>> .ods-modal__dialog--close-animation'); + describe('open', () => { + it('should open on element open method call', async() => { + await setup('Hello, world!'); + const modal = await page.find('ods-modal'); - expect(closeAnimation).not.toBeNull(); + await modal.callMethod('open'); + await page.waitForChanges(); + + const isOpen = await page.evaluate(() => { + const dialog = document.querySelector('ods-modal')?.shadowRoot?.querySelector('.ods-modal__dialog') as HTMLDialogElement; + return dialog && dialog.hasAttribute('open'); + }); + + expect(isOpen).toBe(true); + }); + }); }); }); diff --git a/packages/storybook/stories/components/modal/documentation.mdx b/packages/storybook/stories/components/modal/documentation.mdx index 9f20c46318..8d61106806 100644 --- a/packages/storybook/stories/components/modal/documentation.mdx +++ b/packages/storybook/stories/components/modal/documentation.mdx @@ -62,7 +62,7 @@ They can be dismissable or not. donts={[ '- Trigger a Modal as a result of a first Modal actions', '- Use a Modal if the informational content is very long within a Modal: consider alternatives if that is your case', - - Use a **Modal** only to notify users about a successful completion of their actions: use component, + - Use a Modal only to notify users about a successful completion of their actions: use component, ]} dos={[ '- Use a Modal to display information the user needs to respond to',