[Popper] Upgrade to popper.js to v2#21761
Conversation
| modifiers={{ | ||
| flip: { | ||
| enabled: ${flip}, | ||
| modifiers={[ |
There was a problem hiding this comment.
Modifiers have changed a lot. I didn't want to make any decisions about what to include on this page yet so I included most of the functionality in the flip, preventOverflow and arrow modifiers.
| | 'top-end' | ||
| | 'top-start' | ||
| | 'top'; | ||
| export type PopperPlacementType = ComputedPlacement; |
There was a problem hiding this comment.
I thought it would be nicer to depend on their types. It would be more correct to use Placement instead of ComputedPlacement but I wasn't sure if auto placements were left out on purpose.
There was a problem hiding this comment.
Changed this recently. I'd rather use the correct types
|
For context, I have been collecting resources (started to) around how we could drop Popper in https://trello.com/c/WOj1Oase/2277-remove-popperjs. |
| }, | ||
| }, | ||
| { | ||
| name: 'onUpdate', |
There was a problem hiding this comment.
onUpdate needs to be a custom modifier now
| let popperModifiers = [ | ||
| { | ||
| name: 'preventOverflow', | ||
| options: { |
There was a problem hiding this comment.
Before altBoundary also affected flip but the behaviour is now customisable in both places.
| * To learn how to create a modifier, [read the modifiers documentation](https://popper.js.org/docs/v2/modifiers/). | ||
| */ | ||
| modifiers: PropTypes.object, | ||
| modifiers: PropTypes.arrayOf( |
There was a problem hiding this comment.
Might, be too much info when using the correct types, but I would rather use the correct types and simplify for proptypes.
|
@material-ui/core: parsed: -0.80% 😍, gzip: -0.70% 😍 |
Nice to see the size decrease. We probably could drop this more using popper lite and importing the modifiers we use (https://popper.js.org/docs/v2/tree-shaking/#popper-lite) but we would only not import hide and offset I don't see a massive difference but will try it to see. Nice work @FezVrasta |
| export default function VirtualElementPopper() { | ||
| const [open, setOpen] = React.useState(false); | ||
| const [anchorEl, setAnchorEl] = React.useState<PopperProps['anchorEl']>(null); | ||
| const [anchorEl, setAnchorEl] = React.useState<PopperProps.anchorEl>(null); |
There was a problem hiding this comment.
For some reason, eslint always changes this for me...
There was a problem hiding this comment.
eslint shouldn't touch this file to begin with. What IDE + extension are you using to run eslint on this file?
There was a problem hiding this comment.
Ah looks like it's a webstorm feature:
When checking if ESLint should be run on a .ts files, WebStorm 2018.3 analyzes the root config statically to see if it includes the following plugins:
"parser":"babel-eslint" or
"parser":"typescript-eslint-parser" or
"eslint-plugin-typescript" or
"@typescript-eslint/parser"
I can turn it off by adding typescript files to .eslintignore
There was a problem hiding this comment.
Can't you tell webstorm to ignore it? Though it might resolve itself after #21758
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
As we are investing time in improving the relative positioning logic, we might as well keep the broader picture in mind and consider all our options. The broader choice of options we have, the best decision we can make. I think that now is a great opportunity to look at:
Now, there are longer-term considerations:
|
What issues do you see with the relative position logic apart from #21661? I've done some more investigation locally:
|
27d202b to
e2c6d9c
Compare
eps1lon
left a comment
There was a problem hiding this comment.
There are a couple of tests that could possibly be simplified:
a2a0f1a to
3f921c7
Compare
Makes a lot more sense now after landing #23181:
|
Breaking changes
Upgrade Popper.js from v1 to v2.
This third-party library has introduced a lot of changes.
You can read their migration guide or the following summary:
The CSS prefixes have changed:
popper: { zIndex: 1, - '&[x-placement*="bottom"] $arrow': { + '&[data-popper-placement*="bottom"] $arrow': {Method names have changed.
Modifiers' API has changed a lot. There are too many changes to be covered here.
Just wanted to see what kind of work was needed to migrate and have it run through the CI.
Relevant to: #19358
Closes #19358
Demo: https://deploy-preview-21761--material-ui.netlify.app/components/popper/