Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(modal): calciteModalClose should emit on close button click #7680

Merged
merged 6 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions packages/calcite-components/src/components/modal/modal.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,31 @@ describe("opening and closing behavior", () => {
]);
});

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

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

const beforeCloseSpy = await modal.spyOnEvent("calciteModalBeforeClose");
const closeSpy = await modal.spyOnEvent("calciteModalClose");
const modalBeforeClose = page.waitForEvent("calciteModalBeforeClose");
const modalClose = page.waitForEvent("calciteModalClose");

const closeButton = await page.find("calcite-modal >>> .close");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this use the CSS lookup object?

await closeButton.click();

await modalBeforeClose;
await modalClose;

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
30 changes: 21 additions & 9 deletions packages/calcite-components/src/components/modal/modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,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 @@ -510,10 +510,10 @@ export class Modal
onToggleOpenCloseComponent(this);
if (value) {
this.transitionEl?.classList.add(CSS.openingIdle);
this.openModal();
this.openModal(true);
} else {
this.transitionEl?.classList.add(CSS.closingIdle);
this.closeModal();
this.closeModal(true);
}
}

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

/** Open the modal */
private openModal() {
private handleCloseClick = () => {
this.closeModal();
};

/**
* Open the modal
*
* @param ignoreOpenChange - Ignores the open watcher.
*/
private openModal(ignoreOpenChange = false) {
if (this.ignoreOpenChange) {
return;
}

this.ignoreOpenChange = true;
this.ignoreOpenChange = ignoreOpenChange;
this.el.addEventListener("calciteModalOpen", this.openEnd);
this.open = true;
this.opened = true;
Expand All @@ -554,8 +562,12 @@ export class Modal
this.closeModal();
};

/** Close the modal, first running the `beforeClose` method */
closeModal = async (): Promise<void> => {
/**
* Close the modal, first running the `beforeClose` method
*
* @param ignoreOpenChange - Ignores the open watcher.
*/
closeModal = async (ignoreOpenChange = false): Promise<void> => {
if (this.ignoreOpenChange) {
return;
}
Expand All @@ -574,7 +586,7 @@ export class Modal
}
}

this.ignoreOpenChange = true;
this.ignoreOpenChange = ignoreOpenChange;
this.open = false;
this.opened = false;
this.removeOverflowHiddenClass();
Expand Down
25 changes: 17 additions & 8 deletions packages/calcite-components/src/utils/openCloseComponent.ts
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Elijbet Let's look into refactoring this to follow <propName>Prop, similar to other props, so components can specify their source-of-truth open prop. #proppropprop


/**
* 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
Loading