Skip to content

Commit

Permalink
Feature fix modal cleanup (#44)
Browse files Browse the repository at this point in the history 
* feat(src/modal): use display: none when modal is hidden to avoid showing in some site layout

based on root css, previous implementation might display block layout when hidden. This is addressed by using display: none when hidden and adding transition-behavior, @starting-styles to transition display property to keep the animation looks like original implementation

On some old browsers, the transition animation might not work.

Signed-off-by: James Chien <james@numbersprotocol.io>

* feat: cleanup injected modal automatically when capture-eye is removed from DOM or click on close button

Signed-off-by: James Chien <james@numbersprotocol.io>

* test(src/test): fix tests

Signed-off-by: James Chien <james@numbersprotocol.io>

* fix(src/modal/modal-manager.ts): avoid creating modal when not necessary

Signed-off-by: James Chien <james@numbersprotocol.io>

---------

Signed-off-by: James Chien <james@numbersprotocol.io>
  • Loading branch information
@shc261392
shc261392 authored Dec 5, 2024
1 parent 4169e98 commit bac6335
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 98 deletions.
12 changes: 6 additions & 6 deletions src/capture-eye.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ export class CaptureEye extends LitElement {
console.debug(CaptureEyeModal.name); // The line ensures CaptureEyeModal is included in compilation.
}

override disconnectedCallback() {
super.disconnectedCallback();
ModalManager.getInstance().removeModal();
}

get isOpened() {
return !ModalManager.getInstance().modalHidden;
}
Expand All @@ -99,7 +104,7 @@ export class CaptureEye extends LitElement {
}

close() {
ModalManager.getInstance().hideModal();
ModalManager.getInstance().removeModal();
}

private buttonTemplate() {
Expand Down Expand Up @@ -143,11 +148,6 @@ export class CaptureEye extends LitElement {
`;
}

override async connectedCallback() {
super.connectedCallback();
ModalManager.getInstance().initializeModal();
}

openEye(event?: Event) {
if (event) {
event.stopPropagation();
Expand Down
50 changes: 28 additions & 22 deletions src/modal/modal-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,19 @@ export class ModalManager {
return ModalManager.instance;
}

initializeModal(): void {
if (!this.modalElement) {
this.modalElement = document.createElement('capture-eye-modal');
document.body.appendChild(this.modalElement);
}
}

async updateModal(options: ModalOptions, delay = 150): Promise<void> {
if (!this.modalElement) return;
this.modalElement.modalHidden = true;
let modal = this.getModal();
modal.modalHidden = true;
await new Promise((resolve) => setTimeout(resolve, delay));
const nidChanged = this.modalElement.nid !== options.nid;
const nidChanged = modal.nid !== options.nid;
if (nidChanged) {
this.modalElement.clearModalOptions();
modal.clearModalOptions();
this.removeModal();
modal = this.getModal();
}
this.modalElement.modalHidden = false;
modal.modalHidden = false;
this.registerRootClickListener();
this.modalElement.updateModalOptions(options);
modal.updateModalOptions(options);

if (nidChanged) {
fetchAsset(options.nid).then((assetData) => {
Expand All @@ -68,11 +63,24 @@ export class ModalManager {
}
}

hideModal(): void {
if (this.modalElement) {
this.modalElement.modalHidden = true;
this.unregisterRootClickListener();
removeModal(): void {
if (!this.modalElement) return;
this.modalElement.modalHidden = true;
this.unregisterRootClickListener();
this.modalElement.remove();
this.modalElement = null;
}

private getModal(): CaptureEyeModal {
if (!this.modalElement) {
this.modalElement = document.createElement('capture-eye-modal');
this.modalElement.addEventListener('remove-capture-eye-modal', () => {
this.removeModal();
});
document.body.appendChild(this.modalElement);
return this.modalElement;
}
return this.modalElement;
}

private updateModalAsset(
Expand All @@ -92,11 +100,9 @@ export class ModalManager {
}

private handleRootClick = (event: MouseEvent) => {
if (
this.modalElement &&
!this.modalElement.contains(event.target as Node)
) {
this.hideModal();
const modal = this.getModal();
if (!modal.contains(event.target as Node)) {
this.removeModal();
}
};
}
25 changes: 18 additions & 7 deletions src/modal/modal-styles.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,32 +27,43 @@ export function getModalStyles() {
.modal {
z-index: 1000;
display: flex;
justify-content: flex-start;
align-items: flex-start;
display: none;
opacity: 0;
transform: scale(0.5);
transition: opacity 0.3s ease-in-out, transform 0.3s ease-in;
transition: opacity 0.3s ease-in-out, transform 0.3s ease-in,
display 0.3s ease;
transition-behavior: allow-discrete;
position: absolute;
}
.modal-visible {
display: flex;
opacity: 1;
transform: scale(1);
}
.modal-container {
background-color: var(--background-color);
border-radius: var(--border-radius);
width: 20rem;
box-shadow: var(--box-shadow);
@starting-style {
.modal.modal-visible {
opacity: 0;
transform: scale(0.5);
}
}
.modal-hidden {
display: none;
opacity: 0;
transform: scale(0.5);
}
.modal-container {
background-color: var(--background-color);
border-radius: var(--border-radius);
width: 20rem;
box-shadow: var(--box-shadow);
}
.modal-content {
padding: 12px 24px 12px 24px;
}
Expand Down
50 changes: 13 additions & 37 deletions src/modal/modal.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,12 @@ export class CaptureEyeModal extends LitElement {
this.stopEngagementZoneRotation();
}

override firstUpdated() {
this.updateModalVisibility();
}

override updated(changedProperties: Map<string | number | symbol, unknown>) {
if (changedProperties.has('modalHidden')) {
this.updateModalVisibility();
const closeButton = this.shadowRoot?.querySelector('.close-button');
if (this.modalElement && closeButton && !this.modalHidden) {
this.updateModelPosition(closeButton as HTMLElement);
}
}
}

Expand Down Expand Up @@ -148,35 +147,6 @@ export class CaptureEyeModal extends LitElement {
this._position = undefined;
}

private updateModalVisibility() {
const closeButton = this.shadowRoot?.querySelector('.close-button');
if (this.modalElement && closeButton) {
if (this.modalHidden) {
this.modalElement.classList.add('modal-hidden');
this.modalElement.classList.remove('modal-visible');
closeButton.classList.add('close-button-hidden');
closeButton.classList.remove('close-button-visible');
// Add a transitionend event listener to move the modal off-screen after the animation
this.modalElement.addEventListener(
'transitionend',
() => {
if (this.modalHidden) {
this.modalElement.style.top = '-9999px';
this.modalElement.style.left = '-9999px';
}
},
{ once: true }
);
} else {
this.updateModelPosition(closeButton as HTMLElement);
this.modalElement.classList.remove('modal-hidden');
this.modalElement.classList.add('modal-visible');
closeButton.classList.remove('close-button-hidden');
closeButton.classList.add('close-button-visible');
}
}
}

private remToPixels(rem: number): number {
return (
rem * parseFloat(getComputedStyle(document.documentElement).fontSize)
Expand Down Expand Up @@ -533,7 +503,12 @@ export class CaptureEyeModal extends LitElement {
</div>
</div>
${this.renderEngagementZone()}
<div class="close-button" @click=${this.hideModal}>
<div
class="close-button ${this.modalHidden
? 'close-button-hidden'
: 'close-button-visible'}"
@click=${this.emitRemoveEvent}
>
${generateCaptureEyeCloseSvg(color, size)}
</div>
</div>
Expand All @@ -552,8 +527,9 @@ export class CaptureEyeModal extends LitElement {
}
}

private hideModal() {
this.modalHidden = true;
private emitRemoveEvent() {
// Emit remove event to trigger ModalManager to remove the modal
this.dispatchEvent(new CustomEvent('remove-capture-eye-modal'));
}

private trackEngagement() {
Expand Down
60 changes: 34 additions & 26 deletions src/test/modal_test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { html } from 'lit';
import { fixture, assert, expect } from '@open-wc/testing';
import { fixture, assert, expect, waitUntil } from '@open-wc/testing';
import {
CaptureEyeModal,
formatTxHash,
Expand Down Expand Up @@ -90,9 +90,9 @@ suite('capture-eye-modal', () => {
);
});

test('handles modal visibility', async () => {
test('handles modal visibility correctly', async () => {
const el = await fixture<CaptureEyeModal>(
html`<capture-eye-modal class="modal-hidden"></capture-eye-modal>`
html`<capture-eye-modal></capture-eye-modal>`
);
const modal = el.shadowRoot?.querySelector('.modal');
expect(modal).to.not.be.null;
Expand All @@ -102,38 +102,44 @@ suite('capture-eye-modal', () => {
await el.updateComplete;
expect(modal?.classList.contains('modal-hidden')).to.be.false;
expect(modal?.classList.contains('modal-visible')).to.be.true;

el.modalHidden = true;
await el.updateComplete;
expect(modal?.classList.contains('modal-hidden')).to.be.true;
});

test('calls hideModal() when close button is clicked', async () => {
test('emits remove-capture-eye-modal event when close button is clicked', async () => {
const el = await fixture<CaptureEyeModal>(
html`<capture-eye-modal></capture-eye-modal>`
);
const closeButton = el.shadowRoot?.querySelector(
'.close-button'
) as HTMLElement;
expect(closeButton).to.exist;

const eventSpy = sinon.spy(el, 'dispatchEvent');

el.modalHidden = false;
await el.updateComplete;
closeButton.click();
await el.updateComplete;

const modal = el.shadowRoot?.querySelector('.modal');
expect(modal?.classList.contains('modal-hidden')).to.be.true;
expect(el.modalHidden).to.be.true;
expect(eventSpy).to.have.been.calledWith(
sinon.match
.instanceOf(CustomEvent)
.and(sinon.match.has('type', 'remove-capture-eye-modal'))
);

eventSpy.restore();
});

test('renders engagement image and link correctly', async () => {
const engagementImage =
'https://static-cdn.numbersprotocol.io/capture-eye/capture-ad.png';
const engagementLink = 'https://example.com';

// Fixture to create the CaptureEyeModal component
const el = await fixture<CaptureEyeModal>(html`
<capture-eye-modal></capture-eye-modal>
`);

// Set the modal options with engagement zones
el.updateModalOptions({
nid: '123',
engagementZones: [{ image: engagementImage, link: engagementLink }],
Expand Down Expand Up @@ -299,7 +305,7 @@ suite('capture-eye-modal', () => {
expect((el as any)._position).to.equal(undefined);
});

test('modal visibility toggle works with transition end', async () => {
test('modal visibility toggle works', async () => {
const el = await fixture<CaptureEyeModal>(html`
<capture-eye-modal></capture-eye-modal>
`);
Expand All @@ -309,21 +315,23 @@ suite('capture-eye-modal', () => {
el.modalHidden = false;
await el.updateComplete;

// Check that the modal is visible
expect(modal.style.top).to.not.equal('-9999px');
expect(modal.style.left).to.not.equal('-9999px');
// Wait until the modal becomes visible
await waitUntil(
() => getComputedStyle(modal).display === 'flex',
'Modal should be visible'
);
expect(getComputedStyle(modal).display).to.equal('flex');

// Set the modal to hidden
el.modalHidden = true;
await el.updateComplete;

// Simulate the 'transitionend' event to trigger moving the modal off-screen
modal.dispatchEvent(new Event('transitionend'));
await el.updateComplete;

// Check that the modal is moved off-screen after hiding
expect(modal.style.top).to.equal('-9999px');
expect(modal.style.left).to.equal('-9999px');
// Wait until the modal is hidden
await waitUntil(
() => getComputedStyle(modal).display === 'none',
'Modal should be hidden'
);
expect(getComputedStyle(modal).display).to.equal('none');
});

test('tracks engagement when engagement link is clicked', async () => {
Expand Down Expand Up @@ -389,16 +397,16 @@ suite('capture-eye-modal', () => {
await el.updateComplete;

// Check that the asset details are rendered
const creator = el.shadowRoot?.querySelector('.top-name') as HTMLElement;
expect(creator.innerText).to.equal(assetData.creator);
const creator = el.shadowRoot?.querySelector('.top-name a') as HTMLElement;
expect(creator.textContent?.trim()).to.equal(assetData.creator);

const location = el.shadowRoot?.querySelector(
'.top-info:last-child'
) as HTMLElement;
expect(location.innerText).to.equal(assetData.captureLocation);
expect(location.textContent?.trim()).to.equal(assetData.captureLocation);

const headline = el.shadowRoot?.querySelector('.headline') as HTMLElement;
expect(headline.innerText).to.equal(assetData.headline);
expect(headline.textContent?.trim()).to.equal(assetData.headline);

const img = el.shadowRoot?.querySelector(
'.profile-img'
Expand Down

0 comments on commit bac6335

Please sign in to comment.