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(useDisableBodyScroll): avoid affecting body's height #1279

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

marcoskolodny
Copy link
Collaborator

Issue: Link

Copy link

Size stats

master this branch diff
Total JS 12.1 MB 12.1 MB -40 B
JS without icons 1.95 MB 1.95 MB -40 B
Lib overhead 68.3 kB 68.3 kB 0 B
Lib overhead (gzip) 16.6 kB 16.6 kB 0 B

Copy link

github-actions bot commented Oct 23, 2024

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

@@ -53,7 +53,6 @@ export const useDisableBodyScroll = (disable: boolean): void => {
`top: ${-bodyScrollTop}px;`,
'left: 0px;',
'right: 0px;',
'bottom: 0px;',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need for the bottom: 0px;, because:

  • If the body's height is smaller than the screen, there is no scroll, so it shouldn't be an issue.
  • If there is scroll, the hook already fixes the top position and scrolls to the corresponding vertical position. Setting bottom to 0px doesn't change anything.

The issue is that if we also set bottom to 0, children of the body that have height:100% will be affected by this hook (because when top and bottom values are set, the height of the container becomes well defined, and it will trigger changes in the child).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've tried different scenarios, and everything seems to work fine:

  • Body that has no scroll
  • Body that has scroll
    • Verified case when scroll position is at the top, middle and bottom of the body
  • Verified that resizing the window while blocking the scroll doesn't affect anything, even if the body didn't have scroll before the resize and I make the window very small.

Copy link

github-actions bot commented Oct 23, 2024

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-rkg5kfb41-flows-projects-65bb050e.vercel.app

Built with commit 1804871.
This pull request is being automatically deployed with vercel-action

@marcoskolodny marcoskolodny added this pull request to the merge queue Oct 29, 2024
Merged via the queue into master with commit b68f317 Oct 29, 2024
11 checks passed
@marcoskolodny marcoskolodny deleted the WEB-2088-fix-disable-scroll-hook branch October 29, 2024 12:49
tuentisre pushed a commit that referenced this pull request Nov 4, 2024
# [16.4.0](v16.3.1...v16.4.0) (2024-11-04)

### Bug Fixes

* **TextField:** adjust position of maxLength's screen reader label ([#1283](#1283)) ([fda424e](fda424e))
* **TextField:** avoid right helper text from wrapping, fix spacing and aria label for maxCount text when multiline is true ([#1272](#1272)) ([85fcb31](85fcb31))
* **useDisableBodyScroll:** avoid affecting body's height ([#1279](#1279)) ([b68f317](b68f317))

### Features

* **PhoneNumberFieldLite:** Phone number field with simple formatting to reduce bundle size ([#1276](#1276)) ([a141b97](a141b97))
* **Row:** allow aria-label in informative rows ([#1269](#1269)) ([65b5d42](65b5d42))
* **Rows, Cards, FeedbackScreen, FormFields, Buttons:** add test ids for components and their internal elements ([#1270](#1270)) ([fc63201](fc63201))
* **Spinner:** improve a11y ([#1274](#1274)) ([5267ad5](5267ad5))
* **Switch, RadioButton, Checkbox, ProgressBar, ProgressBarStepped, Slider:** create inverse variant ([#1277](#1277)) ([3129fb9](3129fb9))
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 16.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants