Conversation
| document.addEventListener('touchstart', handleClickOutside); | ||
| document.addEventListener('keydown', handleKeydown); | ||
|
|
||
| const scrollContainer = document.getElementsByClassName(scrollContainerClass)[0] || document.body; |
There was a problem hiding this comment.
I'm not a big fan of using getElementsByClassName to select an element in the DOM with React. I prefer if we use a ref from the main body or just use document.body.
There was a problem hiding this comment.
also to get the first one querySelector > * ;)
There was a problem hiding this comment.
Me too. However, in this case, my thinking is that - we mostly use the same layout components, so the main container is usually <main className="main main-area>, and I really don't want to have to pass ref of that container to every dropdown we create. Getting the main element by query/className I can set the default to main so that we don't need to specify that prop most of the time.
document.body is not really our scroll container, and querySelector is just syntactic sugar in this case.
Any thoughts?
There was a problem hiding this comment.
Another more explicit way I've been thinking about, but which requires a little more effort is to have a wrapper that basically marks the component as scroll container for dropdowns and use it from context as ref.
Maybe something like:
// PublicLayout.js
<PopperScrollContainer>
{(ref) => <main ref={ref} className="main main-area">{children}</main>}
</PopperScrollContainer>
// Dropdown.js
const { placement, position } = usePopper(popperRef, anchorRef, isOpen, {
scrollContainer
});
// usePopper.js
const scrollContainer = usePopperScrollContainer()
[...]
const contentArea = scrollContainer.current || document.body;
[...]
There was a problem hiding this comment.
We did something similar to get the sticky title more easily in the settings app, where a ref to the main element is being provided:
https://github.com/ProtonMail/react-components/blob/master/components/container/SettingsTitle.js
It's not super pretty either, but for now it's ok I guess. To use that in the dropdown we would need to enforce all apps to provide a ref to the main element.
There was a problem hiding this comment.
Nice, didn't know we had that. It's pretty much what I've described, so I'll use it. This will make it more consistent.
There was a problem hiding this comment.
There are a couple of complicated things though with context. It's value would have to be re-defined in several places:
- in most apps it's just main-area, but for instance header, is outside the main area, so it's not part of the context.
- in contacts, we have several scrollable areas - contacts list and contact view.
- modals are outside of any context, so they can't even re-define the ref to be ContentModal (at least I couldn't)
- it affects also tooltips
There was a problem hiding this comment.
Hmm I see.
I think it could make sense to redefine the context for each situation. It should be possible to redefine it for modals also, but I'll investigate that.
Maybe let's wait with this PR a bit.
4 participants
Related: ProtonMail/design-system#206
If dropdown is supposed to be closed when clicking outside, it's removed on scroll also. Should fix: https://github.com/ProtonMail/protonvpn-settings/issues/69