Skip to content

Commit

Permalink
fix(pie-modal): DSW-2676 ensure the page doesn't scroll when the moda…
Browse files Browse the repository at this point in the history
…l is opened (#2152)

* fix(pie-modal): DSW-2676 page scroll to the top when the modal is opened

* fix(pie-modal): DSW-2676 page scroll to the top when the modal is opened

* fix(pie-modal): DSW-2676 update story

* fix(pie-modal): DSW-2676 add changeset

* fix(pie-modal): DSW-2676 ensure full version of modal renders correctly

* fix(pie-modal): DSW-2676 update cookie tests

* fix(pie-modal): DSW-2676 add tests

* fix(pie-modal): DSW-2676 add tests
raoufswe authored Jan 15, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 678283f commit 9d66d1c
Showing 8 changed files with 50 additions and 11 deletions.
6 changes: 6 additions & 0 deletions .changeset/itchy-peas-develop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@justeattakeaway/pie-modal": patch
"pie-storybook": patch
---

[Fixed] - Ensure page doesn't scroll when the modal is opened
3 changes: 2 additions & 1 deletion apps/pie-storybook/stories/pie-modal.stories.ts
Original file line number Diff line number Diff line change
@@ -274,7 +274,8 @@ const FormStoryTemplate = (props: ModalProps) => {

const ScrollablePageStoryTemplate = (props: ModalProps) => html`
${BaseStoryTemplate(props)}
${createScrollablePageHTML()}`;
${createScrollablePageHTML()}
<pie-button id="open-modal-bottom" @click=${toggleModal}>Toggle Modal</pie-button>`;

const FocusableElementsPageStoryTemplate = (props: ModalProps) => html`
${BaseStoryTemplate(props)}
3 changes: 2 additions & 1 deletion apps/pie-storybook/stories/testing/pie-modal.test.stories.ts
Original file line number Diff line number Diff line change
@@ -285,7 +285,8 @@ const FormStoryTemplate = (props: ModalProps) => {

const ScrollablePageStoryTemplate = (props: ModalProps) => html`
${BaseStoryTemplate(props)}
${createScrollablePageHTML()}`;
${createScrollablePageHTML()}
<pie-button id="open-modal-bottom" @click=${toggleModal}>Toggle Modal</pie-button>`;

const FocusToSpecifiedElementStoryTemplate = (props: ModalProps) => html`
${BaseStoryTemplate(props)}
Original file line number Diff line number Diff line change
@@ -20,11 +20,6 @@ test.describe('PieCookieBanner - Visual tests`', () => {

await pieCookieBannerComponent.clickManagePreferencesAction();

// This fixes modal being pushed down in screenshots
await page.$eval('dialog', (el) => {
el.style.top = '20px';
});

await percySnapshot(page, 'PieCookieBanner Manage preferences - Visual Test');
});

4 changes: 0 additions & 4 deletions packages/components/pie-modal/src/index.ts
Original file line number Diff line number Diff line change
@@ -310,10 +310,6 @@ export class PieModal extends RtlMixin(LitElement) implements ModalProps {
*/
private _disableBodyScroll (): void {
if (this._modalScrollContainer) {
// Hack to prevent Safari rendering the modal outside the viewport
// when the body scroll lock is active
if ('scrollTo' in window) window.scrollTo(0, 0);

disableBodyScroll(this._modalScrollContainer);
}
}
2 changes: 2 additions & 0 deletions packages/components/pie-modal/src/modal.scss
Original file line number Diff line number Diff line change
@@ -36,6 +36,8 @@
flex-direction: column;
}

position: fixed;
top: 0;
border-radius: var(--modal-border-radius);
border: none;
box-shadow: var(--modal-elevation);
29 changes: 29 additions & 0 deletions packages/components/pie-modal/test/component/pie-modal.spec.ts
Original file line number Diff line number Diff line change
@@ -436,6 +436,35 @@ test.describe('scrolling logic', () => {
await expect.soft(page.getByText('Top of page copy')).not.toBeInViewport();
await expect(page.getByText('Bottom of page copy')).toBeInViewport();
});

test('Should preserve scroll position after opening and closing the modal', async ({ page }) => {
// Arrange
const modalScrollLockingPage = new ModalScrollLockingPage(page);

const props: ModalProps = {
...sharedProps,
isOpen: false,
};

await modalScrollLockingPage.load(props);

// Act
// Scroll to the bottom of the page
await page.mouse.wheel(0, 5000);

// The mouse.wheel function causes scrolling, but doesn't wait for the scroll to finish before returning.
await page.waitForTimeout(3000);

// opens the modal
await modalScrollLockingPage.openModalFromPageBottom();

// closes the modal
await page.keyboard.press('Escape');

// Assert
await expect.soft(page.getByText('Top of page copy')).not.toBeInViewport();
await expect(page.getByText('Bottom of page copy')).toBeInViewport();
});
});

test.describe('`hasBackButton` prop', () => {
Original file line number Diff line number Diff line change
@@ -5,10 +5,19 @@ import { ModalComponent } from './pie-modal.component.ts';
export class ModalScrollLockingPage extends BasePage {
readonly modalComponent: ModalComponent;
readonly openModalButtonLocator: Locator;
readonly openModalButtonAtPageBottomLocator: Locator;

constructor (page: Page) {
super(page, 'modal--scroll-locking');
this.modalComponent = new ModalComponent(page);
this.openModalButtonLocator = page.locator('#open-modal');
this.openModalButtonAtPageBottomLocator = page.locator('#open-modal-bottom');
}

/**
* Opens the modal by clicking the open modal button on the bottom of the page.
*/
async openModalFromPageBottom () {
await this.openModalButtonAtPageBottomLocator.click();
}
}

0 comments on commit 9d66d1c

Please sign in to comment.