Skip to content

Fixed layout shift issue when Portal popup appears#21895

Merged
minimaluminium merged 3 commits intomainfrom
fix-portal-popup-layout-shift-DES-547
Dec 17, 2024
Merged

Fixed layout shift issue when Portal popup appears#21895
minimaluminium merged 3 commits intomainfrom
fix-portal-popup-layout-shift-DES-547

Conversation

@minimaluminium
Copy link
Copy Markdown
Member

@minimaluminium minimaluminium commented Dec 17, 2024

ref DES-547

  • when Portal popup is opened and the browser scroll bar is visible, it used to make layout shift, because we were hiding the scrollbar
  • now it applies right margin to body element and the trigger button by calculating the scrollbar width only when the browser scroll bar is visible
  • it also preservers the current right margin for those elements and makes the calculation based on that

@minimaluminium minimaluminium requested a review from sagzy December 17, 2024 01:43
Copy link
Copy Markdown
Contributor

@sagzy sagzy left a comment

Choose a reason for hiding this comment

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

LGTM! Added a comment for a tiny optimisation, but feel free to ignore :)

const scrollbarWidth = outer.offsetWidth - inner.offsetWidth;

// Clean up
outer.parentNode.removeChild(outer);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small improvement idea: use clientWidth instead of creating an inner element:

    ...
    document.body.appendChild(div);
    
    const scrollbarWidth = div.offsetWidth - div.clientWidth;
    
    document.body.removeChild(div);
    ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Great idea! Added the optimization here.

@stromfeldt
Copy link
Copy Markdown
Contributor

Thanks for this fix! I mentioned the issue a couple of years ago on the forum (along with my piss-poor fix), but it apparently couldn't be reproduced for some reason. Your code looks far superior than mine, so I'll remove my code from the sites I've inserted it into as soon as the fix goes live. :)

@minimaluminium
Copy link
Copy Markdown
Member Author

@stromfeldt Your suggested solution is very simple and nice, however, the only thing with it was we still wanted to lock the vertical scrolling when the popup is open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants