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.

Sign up for GitHub

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

Jump to bottom

gapMode prop/API is not actually used #65

Closed
cee-chen opened this issue Mar 15, 2023 · 7 comments
Closed

gapMode prop/API is not actually used #65

cee-chen opened this issue Mar 15, 2023 · 7 comments
Assignees
@theKashey

Comments

@cee-chen
Copy link
Contributor

Despite being documented in the main README, setting gapMode="padding" as a prop does not appear to correctly change the CSS applied to the page body to set padding-right instead of margin-right (which the underlying react-remove-scroll-bar library implies it should).

gapMode usage appears to be nonexistent, and its original implementation no longer exists in the codebase.

Demo of broken behavior: https://codesandbox.io/s/romantic-jennings-tv3l31?file=/demo.js:397-444

Notice that even with gapMode="padding" passed, when the focus lock is open, the CSS rendered is still using margin-right:

A bit more context:

Our team could really use support for this, as unfortunately one of our major consumers has width: 100%; min-width: 100% CSS set on their body, which causes margin-right to not work as expected. We don't really have standing to tell them to remove this CSS, so the only other alternative to use padding-right instead (which works correctly with 100% width).

As a workaround we're currently grabbing the --removed-body-scroll-bar-size CSS var and manually setting padding-right, but we'd love to be able to remove this workaround.
@cee-chen cee-chen mentioned this issue Mar 15, 2023
Merged
4 tasks
theKashey added a commit that referenced this issue Mar 16, 2023
@theKashey
Merge pull request #66 from theKashey/wireup-gap
47a0db0 
fix: wire up gapMode, fixes #65
@theKashey
Copy link
Owner

Hey, thank you for the bug report. v3.8.0 has been released with a fix

@cee-chen
Copy link
Contributor Author

Holy cow, that was so fast!! Thanks a million @theKashey, you rock :)

@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 16, 2023 •
edited
Loading

Gah actually, sorry to be annoying, but I'm not sure this is working. I updated the above CodeSandbox link (https://codesandbox.io/s/romantic-jennings-tv3l31?file=/demo.js:397-444) to use 3.8.0 and gapMode is still not working as expected, and instead there's a React error in the DOM which appears to imply that gapMode is being passed to the underlying DOM element:

@theKashey
Copy link
Owner

😭 because problem is now on ReactRemoveScroll side. I need to update relationships between focus-on(supports) -> remove-scroll(🤡) -> remove-scroll-bar(supports).

Works on local due some unpublished updates. I need to develop more reality-related test suites

@theKashey theKashey reopened this Mar 16, 2023
cee-chen added a commit to elastic/kibana that referenced this issue Mar 20, 2023
@cee-chen @kibanamachine
[EUI] Add scrollLock workaround CSS to Kibana's body (#153227)
b665651 
## Summary

The `scrollLock` property on `EuiFocusTrap` "freezes" the width of the
of the body so that the removal/addition of the scrollbar when a full
screen focus trap opens (e.g. a modal, flyout, or other fullscreen mode)
doesn't cause the page width to jump/rerender.

It turns however, that this behavior (which sets a `margin-right` on the
`body` for the width of the scrollbar) does not work as expected in
Kibana due to its existing `width: 100%; min-width: 100%` CSS on its
`body`. As such, we need to temporarily work around this by setting
`padding-right` instead until
theKashey/react-focus-on#65 is fixed.

d59faae can be reverted if not desired in this PR.

### Checklist

- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
nkhristinin pushed a commit to elastic/kibana that referenced this issue Mar 22, 2023
@cee-chen @kibanamachine @nkhristinin
[EUI] Add scrollLock workaround CSS to Kibana's body (#153227)
b1915ae 
## Summary

The `scrollLock` property on `EuiFocusTrap` "freezes" the width of the
of the body so that the removal/addition of the scrollbar when a full
screen focus trap opens (e.g. a modal, flyout, or other fullscreen mode)
doesn't cause the page width to jump/rerender.

It turns however, that this behavior (which sets a `margin-right` on the
`body` for the width of the scrollbar) does not work as expected in
Kibana due to its existing `width: 100%; min-width: 100%` CSS on its
`body`. As such, we need to temporarily work around this by setting
`padding-right` instead until
theKashey/react-focus-on#65 is fixed.

d59faae can be reverted if not desired in this PR.

### Checklist

- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
tsullivan pushed a commit to tsullivan/kibana that referenced this issue Mar 22, 2023
@cee-chen @kibanamachine @tsullivan
[EUI] Add scrollLock workaround CSS to Kibana's body (elastic#153227
6e6489a 
)

## Summary

The `scrollLock` property on `EuiFocusTrap` "freezes" the width of the
of the body so that the removal/addition of the scrollbar when a full
screen focus trap opens (e.g. a modal, flyout, or other fullscreen mode)
doesn't cause the page width to jump/rerender.

It turns however, that this behavior (which sets a `margin-right` on the
`body` for the width of the scrollbar) does not work as expected in
Kibana due to its existing `width: 100%; min-width: 100%` CSS on its
`body`. As such, we need to temporarily work around this by setting
`padding-right` instead until
theKashey/react-focus-on#65 is fixed.

d59faae can be reverted if not desired in this PR.

### Checklist

- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@theKashey theKashey mentioned this issue Mar 31, 2023
Closed
@Haraldson
Copy link

@theKashey Hey! Any progress on the peer dependency to react-remove-scroll, and getting that up to speed?

@theKashey
Copy link
Owner

🫠 time flies. Sorry, too much other stuff fighting for priorities.

@theKashey theKashey self-assigned this Apr 26, 2023
@theKashey
Copy link
Owner

3.8.1 seems to handle the problem accordingly.

There could be some glitches in the scrollbar if one uses gapMode=padding to be addressed at theKashey/react-remove-scroll-bar#36

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

@theKashey theKashey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Haraldson @cee-chen @theKashey