[Popper] Refactor to more commonly known react patterns #16613
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eps1lon
added
new feature
Details of bundle changes.Comparing: 6dc4ca3...97d2ca4
|
oliviertassinari
approved these changes
Jul 16, 2019
| @@ -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), | |||
There was a problem hiding this comment.
I can't remember why it was added in #12085. Let's try without 🤷♂️
There was a problem hiding this comment.
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.
| /** | ||
| * Flips placement if in <body dir="rtl" /> | ||
| * @param {string} placement | ||
| */ | ||
| function flipPlacement(placement) { | ||
| const direction = (typeof window !== 'undefined' && document.body.getAttribute('dir')) || 'ltr'; |
There was a problem hiding this comment.
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 :).
There was a problem hiding this comment.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Makes the controlled/uncontrolled + derived state pattern more obvious which results in one less
createcallback (wasteful popper re-creation) andflipPlacementcall.Videos are taken with
<Popper flip />and we're about to scroll to a position where the placement needs to be flipped. When this happens we used to get this weirdcreate-update-create-updatechain that is now replaced by a continuous update.https://material-ui.com/components/popper/#scroll-playground
PR:
And we even got less shipped code. Hope that makes the implementation more reasonable.