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

[Popper] Refactor to more commonly known react patterns #16613

Merged
merged 2 commits into from
Jul 17, 2019
Merged

[Popper] Refactor to more commonly known react patterns #16613

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
491d93a
[Popper] Fix re-creation when placement flips
eps1lon Jul 16, 2019
97d2ca4
Apply controlled/uncontrolled/derived state pattern
eps1lon Jul 16, 2019
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
30 changes: 19 additions & 11 deletions packages/material-ui/src/Popper/Popper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ import Portal from '../Portal';
import { createChainedFunction } from '../utils/helpers';
import { useForkRef } from '../utils/reactHelpers';

/**
* Flips placement if in <body dir="rtl" />
* @param {string} placement
*/
function flipPlacement(placement) {
const direction = (typeof window !== 'undefined' && document.body.getAttribute('dir')) || 'ltr';
Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's funny. I might have done it this way so we didn't have to bundle the default theme. As of today, we could probably use the useTheme from styles. But anyway, that's outside of the scope :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Was thinking about this as well but would've required a test change. Wanted to be sure it's clear that this is only a refactor. Will follow-up.


Expand Down Expand Up @@ -47,7 +51,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
keepMounted = false,
modifiers,
open,
placement: placementProps = 'bottom',
placement: initialPlacement = 'bottom',
popperOptions = defaultPopperOptions,
popperRef: popperRefProp,
transition = false,
Expand All @@ -65,13 +69,20 @@ const Popper = React.forwardRef(function Popper(props, ref) {
React.useImperativeHandle(popperRefProp, () => popperRef.current, []);

const [exited, setExited] = React.useState(!open);
const [placement, setPlacement] = React.useState();

const rtlPlacement = flipPlacement(initialPlacement);
/**
* placement initialized from prop but can change during lifetime if modifiers.flip.
* modifiers.flip is essentially a flip for controlled/uncontrolled behavior
*/
const [placement, setPlacement] = React.useState(rtlPlacement);
if (rtlPlacement !== placement) {
setPlacement(rtlPlacement);
}

const handleOpen = React.useCallback(() => {
const handlePopperUpdate = data => {
if (data.placement !== placement) {
setPlacement(data.placement);
}
setPlacement(data.placement);
};

const popperNode = tooltipRef.current;
Expand All @@ -86,7 +97,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
}

const popper = new PopperJS(getAnchorEl(anchorEl), popperNode, {
placement: flipPlacement(placementProps),
placement: rtlPlacement,
...popperOptions,
modifiers: {
...(disablePortal
Expand All @@ -102,11 +113,10 @@ const Popper = React.forwardRef(function Popper(props, ref) {
},
// We could have been using a custom modifier like react-popper is doing.
// But it seems this is the best public API for this use case.
onCreate: createChainedFunction(handlePopperUpdate, popperOptions.onCreate),
Copy link
Member

Choose a reason for hiding this comment

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

I can't remember why it was added in #12085. Let's try without 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

In RTL you create the placement with flipPlacement so when you change placement in an RTL environment you only create a new PopperJS which wouldn't update the child prop. Since it's now immediately derived we no longer need this line.

onUpdate: createChainedFunction(handlePopperUpdate, popperOptions.onUpdate),
});
handlePopperRefRef.current(popper);
}, [anchorEl, disablePortal, modifiers, open, placement, placementProps, popperOptions]);
}, [anchorEl, disablePortal, modifiers, open, rtlPlacement, popperOptions]);

const handleEnter = () => {
setExited(false);
Expand Down Expand Up @@ -148,9 +158,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
return null;
}

const childProps = {
placement: placement || flipPlacement(placementProps),
};
const childProps = { placement };

if (transition) {
childProps.TransitionProps = {
Expand Down