Skip to content

Commit

Permalink
fix(modal): calciteModalClose should emit on close button click (#7680)
Browse files Browse the repository at this point in the history 
**Related Issue:** #7655

## Summary

- Updates openCloseComponent helper to consider `opened` property for
components that have it
- Update modal to emit correctly on close click
- Add test
- @jcfranco for risk consideration.
  • Loading branch information
@driskull
driskull authored Sep 6, 2023
1 parent c3261f0 commit 796bf90
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 34 deletions.
55 changes: 52 additions & 3 deletions packages/calcite-components/src/components/modal/modal.e2e.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe("calcite-modal properties", () => {
const modal = await page.find("calcite-modal");
modal.setProperty("closeButtonDisabled", true);
await page.waitForChanges();
const closeButton = await page.find("calcite-modal >>> .close");
const closeButton = await page.find(`calcite-modal >>> .${CSS.close}`);
expect(closeButton).toBe(null);
});

Expand Down Expand Up @@ -298,6 +298,55 @@ describe("opening and closing behavior", () => {
]);
});

it("emits when closing on click", async () => {
const page = await newE2EPage();
await page.setContent(html`<calcite-modal></calcite-modal>`);
const modal = await page.find("calcite-modal");

const beforeOpenSpy = await modal.spyOnEvent("calciteModalBeforeOpen");
const openSpy = await modal.spyOnEvent("calciteModalOpen");
const beforeCloseSpy = await modal.spyOnEvent("calciteModalBeforeClose");
const closeSpy = await modal.spyOnEvent("calciteModalClose");

expect(beforeOpenSpy).toHaveReceivedEventTimes(0);
expect(openSpy).toHaveReceivedEventTimes(0);
expect(beforeCloseSpy).toHaveReceivedEventTimes(0);
expect(closeSpy).toHaveReceivedEventTimes(0);

expect(await modal.isVisible()).toBe(false);

const modalBeforeOpen = page.waitForEvent("calciteModalBeforeOpen");
const modalOpen = page.waitForEvent("calciteModalOpen");
modal.setProperty("open", true);
await page.waitForChanges();

await modalBeforeOpen;
await modalOpen;

expect(beforeOpenSpy).toHaveReceivedEventTimes(1);
expect(openSpy).toHaveReceivedEventTimes(1);
expect(beforeCloseSpy).toHaveReceivedEventTimes(0);
expect(closeSpy).toHaveReceivedEventTimes(0);

expect(await modal.isVisible()).toBe(true);

const modalBeforeClose = page.waitForEvent("calciteModalBeforeClose");
const modalClose = page.waitForEvent("calciteModalClose");
const closeButton = await page.find(`calcite-modal >>> .${CSS.close}`);
await closeButton.click();
await page.waitForChanges();

await modalBeforeClose;
await modalClose;

expect(beforeOpenSpy).toHaveReceivedEventTimes(1);
expect(openSpy).toHaveReceivedEventTimes(1);
expect(beforeCloseSpy).toHaveReceivedEventTimes(1);
expect(closeSpy).toHaveReceivedEventTimes(1);

expect(await modal.isVisible()).toBe(false);
});

it("emits when set to open on initial render", async () => {
const page = await newProgrammaticE2EPage();

Expand Down Expand Up @@ -474,7 +523,7 @@ describe("calcite-modal accessibility checks", () => {
const createModalHTML = (contentHTML?: string, attrs?: string) =>
`<calcite-modal open ${attrs}>${contentHTML}</calcite-modal>`;

const closeButtonTargetSelector = ".close";
const closeButtonTargetSelector = `.${CSS.close}`;
const focusableContentTargetClass = "test";

const focusableContentHTML = html`<h3 slot="header">Title</h3>
Expand Down Expand Up @@ -543,7 +592,7 @@ describe("calcite-modal accessibility checks", () => {
modal.setProperty("open", true);
await page.waitForChanges();
expect(await modal.isVisible()).toBe(true);
const closeButton = await page.find("calcite-modal >>> .close");
const closeButton = await page.find(`calcite-modal >>> .${CSS.close}`);
await closeButton.click();
await page.waitForChanges();
expect(await modal.isVisible()).toBe(false);
Expand Down
40 changes: 17 additions & 23 deletions packages/calcite-components/src/components/modal/modal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ export class Modal
setUpLoadableComponent(this);
// when modal initially renders, if active was set we need to open as watcher doesn't fire
if (this.open) {
onToggleOpenCloseComponent(this);
requestAnimationFrame(() => this.openModal());
}
}
Expand Down Expand Up @@ -290,7 +289,7 @@ export class Modal
aria-label={this.messages.close}
class={CSS.close}
key="button"
onClick={this.closeModal}
onClick={this.handleCloseClick}
title={this.messages.close}
// eslint-disable-next-line react/jsx-sort-props -- ref should be last so node attrs/props are in sync (see https://github.com/Esri/calcite-design-system/pull/6530)
ref={(el) => (this.closeButtonEl = el)}
Expand Down Expand Up @@ -405,7 +404,7 @@ export class Modal
@Listen("keydown", { target: "window" })
handleEscape(event: KeyboardEvent): void {
if (this.open && !this.escapeDisabled && event.key === "Escape" && !event.defaultPrevented) {
this.closeModal();
this.open = false;
event.preventDefault();
}
}
Expand Down Expand Up @@ -502,18 +501,25 @@ export class Modal
}

@Watch("open")
async toggleModal(value: boolean): Promise<void> {
toggleModal(value: boolean): void {
if (this.ignoreOpenChange) {
return;
}

if (value) {
this.openModal();
} else {
this.closeModal();
}
}

@Watch("opened")
handleOpenedChange(value: boolean): void {
onToggleOpenCloseComponent(this);
if (value) {
this.transitionEl?.classList.add(CSS.openingIdle);
this.openModal();
} else {
this.transitionEl?.classList.add(CSS.closingIdle);
this.closeModal();
}
}

Expand All @@ -522,15 +528,12 @@ export class Modal
this.el.removeEventListener("calciteModalOpen", this.openEnd);
};

/** Open the modal */
private openModal() {
if (this.ignoreOpenChange) {
return;
}
private handleCloseClick = () => {
this.open = false;
};

this.ignoreOpenChange = true;
private openModal() {
this.el.addEventListener("calciteModalOpen", this.openEnd);
this.open = true;
this.opened = true;
const titleEl = getSlotted(this.el, SLOTS.header);
const contentEl = getSlotted(this.el, SLOTS.content);
Expand All @@ -543,23 +546,17 @@ export class Modal
// use an inline style instead of a utility class to avoid global class declarations.
document.documentElement.style.setProperty("overflow", "hidden");
}
this.ignoreOpenChange = false;
}

private handleOutsideClose = (): void => {
if (this.outsideCloseDisabled) {
return;
}

this.closeModal();
this.open = false;
};

/** Close the modal, first running the `beforeClose` method */
closeModal = async (): Promise<void> => {
if (this.ignoreOpenChange) {
return;
}

if (this.beforeClose) {
try {
await this.beforeClose(this.el);
Expand All @@ -574,11 +571,8 @@ export class Modal
}
}

this.ignoreOpenChange = true;
this.open = false;
this.opened = false;
this.removeOverflowHiddenClass();
this.ignoreOpenChange = false;
};

private removeOverflowHiddenClass(): void {
Expand Down
21 changes: 21 additions & 0 deletions packages/calcite-components/src/demos/modal.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1092,6 +1092,22 @@ <h3 slot="header">Test custom sizes</h3>
Custom width and height preview
</calcite-button>
</div>

<!--
**************************************************
* beforeClose rejected
**************************************************
-->
<div class="child">
<calcite-modal id="js-modal-before-close">
<h3 slot="header">Test rejected beforeClose</h3>
<div slot="content">test</div>
<calcite-button slot="secondary" width="full" appearance="outline">Cancel</calcite-button>
</calcite-modal>
<calcite-button onClick="openModal('#js-modal-before-close')" appearance="outline" scale="s">
beforeClose rejected
</calcite-button>
</div>
</div>

<!-- Magic code that makes every calcite link work -->
Expand All @@ -1100,6 +1116,11 @@ <h3 slot="header">Test custom sizes</h3>
const heightInput = document.querySelector("#css-modal-height-adjuster");
const widthInput = document.querySelector("#css-modal-width-adjuster");
const optionsSegmentedControl = document.querySelector("#css-modal-options-adjuster");
const beforeCloseRejected = document.getElementById("js-modal-before-close");

beforeCloseRejected.beforeClose = () => {
return new Promise((_resolve, reject) => setTimeout(reject, 300));
};

heightInput.addEventListener("calciteInputInput", (event) => {
customSizeModal.style.setProperty("--calcite-modal-height", event.target.value);
Expand Down
25 changes: 17 additions & 8 deletions packages/calcite-components/src/utils/openCloseComponent.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ export interface OpenCloseComponent {
*/
open?: boolean;

/**
* When true, the component is open.
*/
opened?: boolean;

/**
* Specifies the name of transitionProp.
*/
Expand Down Expand Up @@ -55,22 +60,26 @@ const componentToTransitionListeners = new WeakMap<
[HTMLDivElement, typeof transitionStart, typeof transitionEnd]
>();

function transitionStart(event: TransitionEvent): void {
function transitionStart(this: OpenCloseComponent, event: TransitionEvent): void {
if (event.propertyName === this.openTransitionProp && event.target === this.transitionEl) {
this.open ? this.onBeforeOpen() : this.onBeforeClose();
isOpen(this) ? this.onBeforeOpen() : this.onBeforeClose();
}
}
function transitionEnd(event: TransitionEvent): void {
function transitionEnd(this: OpenCloseComponent, event: TransitionEvent): void {
if (event.propertyName === this.openTransitionProp && event.target === this.transitionEl) {
this.open ? this.onOpen() : this.onClose();
isOpen(this) ? this.onOpen() : this.onClose();
}
}

function isOpen(component: OpenCloseComponent): boolean {
return "opened" in component ? component.opened : component.open;
}

function emitImmediately(component: OpenCloseComponent, nonOpenCloseComponent = false): void {
(nonOpenCloseComponent ? component[component.transitionProp] : component.open)
(nonOpenCloseComponent ? component[component.transitionProp] : isOpen(component))
? component.onBeforeOpen()
: component.onBeforeClose();
(nonOpenCloseComponent ? component[component.transitionProp] : component.open)
(nonOpenCloseComponent ? component[component.transitionProp] : isOpen(component))
? component.onOpen()
: component.onClose();
}
Expand Down Expand Up @@ -131,15 +140,15 @@ export function onToggleOpenCloseComponent(component: OpenCloseComponent, nonOpe
if (event.propertyName === component.openTransitionProp && event.target === component.transitionEl) {
clearTimeout(fallbackTimeoutId);
component.transitionEl.removeEventListener("transitionstart", onStart);
(nonOpenCloseComponent ? component[component.transitionProp] : component.open)
(nonOpenCloseComponent ? component[component.transitionProp] : isOpen(component))
? component.onBeforeOpen()
: component.onBeforeClose();
}
}

function onEndOrCancel(event: TransitionEvent): void {
if (event.propertyName === component.openTransitionProp && event.target === component.transitionEl) {
(nonOpenCloseComponent ? component[component.transitionProp] : component.open)
(nonOpenCloseComponent ? component[component.transitionProp] : isOpen(component))
? component.onOpen()
: component.onClose();

Expand Down

0 comments on commit 796bf90

Please sign in to comment.