Skip to content
This repository was archived by the owner on Aug 11, 2021. It is now read-only.

Hide dropdown on scroll #223

Open
vincaslt wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Hide dropdown on scroll #223

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions components/dropdown/Dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import React, { useEffect, useRef } from 'react';
import React, { useEffect, useRef, useCallback } from 'react';
import PropTypes from 'prop-types';
import keycode from 'keycode';
import { classnames } from '../../helpers/component';
import { usePopper, Popper } from '../popper';
import useRightToLeft from '../../containers/rightToLeft/useRightToLeft';
import { noop } from 'proton-shared/lib/helpers/function';

const scrollContainerClass = 'main';

const Dropdown = ({
anchorRef,
children,
Expand All @@ -27,18 +29,18 @@ const Dropdown = ({
const { placement, position } = usePopper(popperRef, anchorRef, isOpen, {
originalPlacement: isRTL ? rtlAdjustedPlacement : originalPlacement,
offset: 20,
scrollContainerClass: 'main'
scrollContainerClass
});

const handleKeydown = (event) => {
const handleKeydown = useCallback((event) => {
const key = keycode(event);

if (key === 'escape' && event.target === document.activeElement) {
onClose();
}
};
}, []);

const handleClickOutside = (event) => {
const handleClickOutside = useCallback((event) => {
// Do nothing if clicking ref's element or descendent elements
if (
!autoCloseOutside ||
Expand All @@ -48,7 +50,13 @@ const Dropdown = ({
return;
}
onClose();
};
}, []);

const handleScroll = useCallback(() => {
if (autoCloseOutside) {
onClose();
}
}, []);

const handleClickContent = () => {
if (autoClose) {
Expand All @@ -61,10 +69,13 @@ const Dropdown = ({
document.addEventListener('touchstart', handleClickOutside);
document.addEventListener('keydown', handleKeydown);

const scrollContainer = document.getElementsByClassName(scrollContainerClass)[0] || document.body;
Copy link
Contributor

@EpokK EpokK Aug 26, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dhoko dhoko Aug 26, 2019

Choose a reason for hiding this comment

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

also to get the first one querySelector > * ;)

Copy link
Contributor Author

@vincaslt vincaslt Aug 27, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@vincaslt vincaslt Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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;
[...]

Copy link
Member

@mmso mmso Aug 27, 2019

Choose a reason for hiding this comment

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

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/protonmail-settings/blob/master/src/app/components/layout/PrivateLayout.js#L30

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.

Copy link
Contributor Author

@vincaslt vincaslt Aug 27, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vincaslt vincaslt Aug 28, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

@mmso mmso Aug 28, 2019

Choose a reason for hiding this comment

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

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.

scrollContainer.addEventListener('scroll', handleScroll);
return () => {
document.removeEventListener('mousedown', handleClickOutside);
document.removeEventListener('touchstart', handleClickOutside);
document.removeEventListener('keydown', handleKeydown);
scrollContainer.removeEventListener('scroll', handleScroll);
};
}, []);

Expand Down