search

theKashey / react-focus-on

🎯 Solution for WAI ARIA compatible modal dialogs or full-screen tasks, you were looking for
MIT License
333 stars 14 forks source link

`gapMode` prop/API is not actually used #65

Closed cee-chen closed 1 year ago

cee-chen commented 1 year ago

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.

theKashey commented 1 year ago

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

cee-chen commented 1 year ago

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

cee-chen commented 1 year ago

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 commented 1 year ago

😭 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

Haraldson commented 1 year ago

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

theKashey commented 1 year ago

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

theKashey commented 1 year ago

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 https://github.com/theKashey/react-remove-scroll-bar/pull/36