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.

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

[Popper] Fix deep merge of PopperProps #19851

Merged
merged 8 commits into from
Feb 29, 2020
Merged

[Popper] Fix deep merge of PopperProps #19851

merged 8 commits into from
Feb 29, 2020

Conversation

valgrindMaster
Copy link
Contributor

@valgrindMaster valgrindMaster commented Feb 25, 2020

Problem

The tooltip arrow positioning was incorrect after passing custom PopperProps to the Tooltip component. We want to pass custom Popper modifiers into the underlying popper.js instance, but could not do so without causing the tooltip arrow positioning to behave unexpectedly.

Investigation

We discovered that the arrow modifier that gets passed to popperOptions was being overwritten as a result of passing anything else in popperOptions.

Solution

Deep-merged the default popperOptions arrow modifier with the custom modifiers passed in by the user.

@valgrindMaster valgrindMaster changed the title Merge modifiers within popperOptions without breaking the arrow posit… Merge modifiers within popperOptions without breaking the arrow positioning. Feb 25, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Feb 25, 2020

Details of bundle changes.

Comparing: aa09632...0c7d2bb

bundle Size Change Size Gzip Change Gzip
docs.main ▲ +54 B (+0.01% ) 601 kB ▲ +26 B (+0.01% ) 195 kB
@material-ui/lab ▲ +50 B (+0.03% ) 196 kB ▲ +41 B (+0.07% ) 58.2 kB
@material-ui/core ▲ +50 B (+0.01% ) 359 kB ▲ +25 B (+0.03% ) 98.9 kB
SpeedDialAction ▲ +50 B (+0.04% ) 119 kB ▲ +22 B (+0.06% ) 37.6 kB
Tooltip ▲ +50 B (+0.05% ) 102 kB ▲ +14 B (+0.04% ) 32.5 kB
@material-ui/core[umd] ▲ +31 B (+0.01% ) 318 kB ▼ -5 B (-0.01% ) 92.2 kB
@material-ui/styles -- 51.4 kB -- 15.4 kB
@material-ui/system -- 16.5 kB -- 4.32 kB
Alert -- 83.6 kB -- 26.4 kB
AlertTitle -- 64.5 kB -- 20.3 kB
AppBar -- 64.3 kB -- 20.2 kB
Autocomplete -- 132 kB -- 41.5 kB
Avatar -- 65.5 kB -- 20.7 kB
AvatarGroup -- 62.7 kB -- 19.7 kB
Backdrop -- 68.2 kB -- 21.1 kB
Badge -- 65.6 kB -- 20.4 kB
BottomNavigation -- 62.7 kB -- 19.7 kB
BottomNavigationAction -- 75.8 kB -- 24 kB
Box -- 72.2 kB -- 21.9 kB
Breadcrumbs -- 80.6 kB -- 25.4 kB
Button -- 80 kB -- 24.6 kB
ButtonBase -- 74.3 kB -- 23.4 kB
ButtonGroup -- 83.4 kB -- 25.6 kB
Card -- 63.2 kB -- 19.8 kB
CardActionArea -- 75.3 kB -- 23.8 kB
CardActions -- 62.4 kB -- 19.6 kB
CardContent -- 62.3 kB -- 19.5 kB
CardHeader -- 65.4 kB -- 20.6 kB
CardMedia -- 62.7 kB -- 19.7 kB
Checkbox -- 83.1 kB -- 26.3 kB
Chip -- 82.8 kB -- 25.4 kB
CircularProgress -- 64.4 kB -- 20.3 kB
ClickAwayListener -- 3.84 kB -- 1.54 kB
Collapse -- 68.3 kB -- 21.2 kB
colorManipulator -- 3.88 kB -- 1.52 kB
Container -- 63.5 kB -- 19.9 kB
CssBaseline -- 62.3 kB -- 19.6 kB
Dialog -- 83.2 kB -- 25.9 kB
DialogActions -- 62.4 kB -- 19.6 kB
DialogContent -- 62.5 kB -- 19.6 kB
DialogContentText -- 64.4 kB -- 20.2 kB
DialogTitle -- 64.6 kB -- 20.3 kB
Divider -- 63 kB -- 19.8 kB
docs.landing -- 56.6 kB -- 15.6 kB
Drawer -- 84.9 kB -- 25.9 kB
ExpansionPanel -- 72.6 kB -- 22.7 kB
ExpansionPanelActions -- 62.4 kB -- 19.6 kB
ExpansionPanelDetails -- 62.3 kB -- 19.5 kB
ExpansionPanelSummary -- 78.3 kB -- 24.8 kB
Fab -- 77.1 kB -- 24.1 kB
Fade -- 23.6 kB -- 8.01 kB
FilledInput -- 73.9 kB -- 23 kB
FormControl -- 64.7 kB -- 20.2 kB
FormControlLabel -- 65.8 kB -- 20.7 kB
FormGroup -- 62.3 kB -- 19.5 kB
FormHelperText -- 63.7 kB -- 20 kB
FormLabel -- 63.8 kB -- 19.8 kB
Grid -- 65.4 kB -- 20.5 kB
GridList -- 62.8 kB -- 19.7 kB
GridListTile -- 64 kB -- 20.1 kB
GridListTileBar -- 63.5 kB -- 19.9 kB
Grow -- 24.2 kB -- 8.22 kB
Hidden -- 66.3 kB -- 20.8 kB
Icon -- 63.1 kB -- 19.8 kB
IconButton -- 76.4 kB -- 23.9 kB
Input -- 72.9 kB -- 22.8 kB
InputAdornment -- 65.4 kB -- 20.6 kB
InputBase -- 71 kB -- 22.3 kB
InputLabel -- 65.6 kB -- 20.5 kB
LinearProgress -- 65.6 kB -- 20.5 kB
Link -- 66.9 kB -- 21.1 kB
List -- 62.7 kB -- 19.6 kB
ListItem -- 77.3 kB -- 24.3 kB
ListItemAvatar -- 62.4 kB -- 19.6 kB
ListItemIcon -- 62.5 kB -- 19.6 kB
ListItemSecondaryAction -- 62.3 kB -- 19.5 kB
ListItemText -- 65.3 kB -- 20.5 kB
ListSubheader -- 63.1 kB -- 19.8 kB
Menu -- 88.8 kB -- 27.5 kB
MenuItem -- 78.4 kB -- 24.6 kB
MenuList -- 66.3 kB -- 20.8 kB
MobileStepper -- 68.1 kB -- 21.4 kB
Modal -- 14.3 kB -- 5.03 kB
NativeSelect -- 77.1 kB -- 24.4 kB
NoSsr -- 2.17 kB -- 1.03 kB
OutlinedInput -- 74.9 kB -- 23.4 kB
Pagination -- 85.3 kB -- 26.3 kB
PaginationItem -- 81 kB -- 25 kB
Paper -- 62.7 kB -- 19.6 kB
Popover -- 83.2 kB -- 25.8 kB
Popper -- 28.8 kB -- 10.3 kB
Portal -- 2.87 kB -- 1.29 kB
Radio -- 84.1 kB -- 26.6 kB
RadioGroup -- 64.7 kB -- 20.1 kB
Rating -- 70.7 kB -- 22.7 kB
RootRef -- 4.21 kB -- 1.63 kB
ScopedCssBaseline -- 63.1 kB -- 19.9 kB
Select -- 116 kB -- 34.7 kB
Skeleton -- 63.3 kB -- 20 kB
Slide -- 25.6 kB -- 8.74 kB
Slider -- 76.9 kB -- 24.2 kB
Snackbar -- 75.6 kB -- 23.7 kB
SnackbarContent -- 63.8 kB -- 20.1 kB
SpeedDial -- 86.4 kB -- 27.3 kB
SpeedDialIcon -- 64.9 kB -- 20.3 kB
Step -- 63 kB -- 19.8 kB
StepButton -- 82.5 kB -- 26.2 kB
StepConnector -- 63 kB -- 19.8 kB
StepContent -- 69.5 kB -- 21.8 kB
StepIcon -- 64.9 kB -- 20.3 kB
StepLabel -- 68.9 kB -- 21.7 kB
Stepper -- 65.2 kB -- 20.6 kB
styles/createMuiTheme -- 16.6 kB -- 5.85 kB
SvgIcon -- 63.4 kB -- 19.8 kB
SwipeableDrawer -- 92.3 kB -- 28.9 kB
Switch -- 82.3 kB -- 26 kB
Tab -- 76.6 kB -- 24.3 kB
Table -- 62.9 kB -- 19.7 kB
TableBody -- 62.4 kB -- 19.5 kB
TableCell -- 64.3 kB -- 20.3 kB
TableContainer -- 62.3 kB -- 19.5 kB
TableFooter -- 62.4 kB -- 19.5 kB
TableHead -- 62.4 kB -- 19.5 kB
TablePagination -- 143 kB -- 42 kB
TableRow -- 62.8 kB -- 19.7 kB
TableSortLabel -- 77.6 kB -- 24.4 kB
Tabs -- 85.5 kB -- 27.2 kB
TextareaAutosize -- 5.19 kB -- 2.17 kB
TextField -- 125 kB -- 36.7 kB
ToggleButton -- 76.4 kB -- 24.2 kB
ToggleButtonGroup -- 63.5 kB -- 20 kB
Toolbar -- 62.7 kB -- 19.7 kB
TreeItem -- 75.5 kB -- 23.9 kB
TreeView -- 69.2 kB -- 21.7 kB
Typography -- 64 kB -- 20 kB
useAutocomplete -- 14.7 kB -- 5.33 kB
useMediaQuery -- 2.56 kB -- 1.06 kB
Zoom -- 23.6 kB -- 8.13 kB

Generated by 🚫 dangerJS against 0c7d2bb

@eps1lon eps1lon changed the title Merge modifiers within popperOptions without breaking the arrow positioning. [Popper] Merge modifiers within popperOptions without breaking the arrow positioning. Feb 26, 2020
@eps1lon eps1lon added bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. labels Feb 26, 2020
@@ -397,6 +397,68 @@ describe('<Tooltip />', () => {
});
});

describe('prop: PopperProps', () => {
it('should pass PopperProps to Popper Component', () => {
const wrapper = mount(<Tooltip {...defaultProps} PopperProps={{ item: 'value' }} />);
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of using the render API instead (createClientRender)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent over 45 minutes attempting to use createClientRender, sadly I don't have anymore time to work on this. I propose that we go with the unit tests that I've provided, and feel free to update them yourself if you'd rather use that approach.

Copy link
Member

@oliviertassinari oliviertassinari Feb 26, 2020

Choose a reason for hiding this comment

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

We are progressively moving away from enzyme, I think that any new tests using it should be a no go.

Copy link
Member

Choose a reason for hiding this comment

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

We might look into it.

modifiers: {
arrow: {
enabled: Boolean(arrowRef),
element: arrowRef,
},
...(popperOptions && popperOptions.modifiers),
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't if we shouldn't be using a deep merge logic (deepmerge.js), per the arrow option people might want to override: https://popper.js.org/docs/v2/modifiers/arrow/#padding (but this is v2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the switch to using deepmerge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the commit. deepmerge does not work with DOM elements. arrowRef was causing an infinite recursive loop. I did however add manual merging of default and custom arrow to allow for what you are asking. I do not have the time to look into refactoring deepmerge to accommodate this broken usecase. How about I make an issue to support DOM elements in deepmerge and to switch over to using it at that time?

Copy link
Member

Choose a reason for hiding this comment

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

You didn't use the right deepmerge module. Our version should already support DOM elements.

Copy link
Contributor

@jkjustjoshing jkjustjoshing Feb 26, 2020

Choose a reason for hiding this comment

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

Can you clarify which deepmerge module we should be using (I worked with @valgrindMaster on this PR)?
We tried import deepmerge from 'deepmerge'

Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 26, 2020
@oliviertassinari oliviertassinari changed the title [Popper] Merge modifiers within popperOptions without breaking the arrow positioning. [Popper] Fix deep merge of PopperProps Feb 28, 2020
@oliviertassinari oliviertassinari added PR: ready to ship and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 29, 2020
@oliviertassinari oliviertassinari merged commit a013f2e into mui:master Feb 29, 2020
@oliviertassinari
Copy link
Member

@valgrindMaster Thanks for raising this issue and working on it! :)

@jkjustjoshing
Copy link
Contributor

Thanks @oliviertassinari for helping us out with those unit tests, and for such a quick turnaround with the 4.9.5 release! Very much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants