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

Feature request: onOverlayChange prop #103

Open
minisemi opened this issue Jul 26, 2024 · 7 comments
Open

Feature request: onOverlayChange prop #103

minisemi opened this issue Jul 26, 2024 · 7 comments

Comments

@minisemi
Copy link

When using embedded components in general, some will open up an overlay window (like the one to the right in my screenshot). That overlay will have its own scrollbar. Since the page has its own scrollbar, there will now be two scrollbars on the page, as seen in the screenshot (the two grey vertical bars to the far right). This leads to a buggy interaction for the user, since sometimes scrolling will scroll the page behind the overlay and sometimes it will scroll the overlay itself. I therefore request adding a prop to all embedded components called e.g. onOverlayChange which will receive a function. The onOverlayChange should then be called each time the embedded component opens or closes an overlay, returning a true or false value depending on if it closed or opened an overlay. We developers can then through that onOverlayChange function turn on/off the scrollbar on the page behind the overlay, so the overlay is the only scrollable element on the page when it is open.

image

@jorgea-stripe
Copy link
Contributor

Hey @minisemi , do you have a repro for this (maybe a small repository where we'd be able to see this happening)?

@minisemi
Copy link
Author

@jorgea-stripe This is a feature request, i.e I assumed the embedded components didn't try to disable the scroll on the page themselves. In that case you can reproduce this behaviour on any setup.

But if you are saying the embedded components are meant to disable the scroll on the page, i.e. I should label this as a bug instead, then I can try to make small repro for this. And then I would also like to know where in the code react-connect-js is trying to do this so I can take a look, because after inspecting the html and css of the and tags of the page I can't find any traces of react-connect-js trying to change any css when opening an overlay.

@jorgea-stripe
Copy link
Contributor

Yes, I believe this is a bug on our side. They are meant to disable scrolling on the page, however this doesn't appear to be working as expected on the "drawer" option.

Note the actual code that does this isn't on these repo's, as these are pure loaders for connect.js.

@minisemi
Copy link
Author

Ah okey. Hmm it isn't working for me without the drawer option either. We disable scroll on the tag, so maybe the repo only disables it on the tag, but I couldn't find traces of it editing the css of the tag either when the overlay opens.

@jorgea-stripe
Copy link
Contributor

Right, the code that does that isn't on this repo. The reason I am asking for a repro is that this doesn't happen generally, for example check out the payments embedded components demo in the doc site: https://docs.stripe.com/connect/supported-embedded-components/payments - you can see there we do disable scrolling. I believe this happens in some pages that don't use the body for scrolling, which is why a repro would be helpful

image

@minisemi
Copy link
Author

minisemi commented Aug 7, 2024

The page on the link you provided doesn't use the body for scrolling. It uses the div with class "Context Box-root". I can't see that it disables any scrolling using CSS though, on either the html, body or the "Context Box-root"-div, because none of their CSS is changed when opening/closing the stripe popup. So perhaps it uses javascript to disable scrolling instead.

I made a quick codesandbox here to display our use case. I don't know how to get the stripeConnectInstance since it requires backend though, but you get the idea.

https://codesandbox.io/s/laughing-bush-nf9jns?file=/src/App.js

@minisemi
Copy link
Author

@jorgea-stripe Any update on this?

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

No branches or pull requests

2 participants