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

[Autocomplete] added new prop called ListOptionComponent #19235

Closed
wants to merge 1 commit into from

Conversation

sherodtaylor
Copy link

@sherodtaylor sherodtaylor commented Jan 14, 2020

I found an issue with the implementation of autocomplete. If you look at the PR you generate the props from the useAutocomplete hook and pass that directly to the li. This is an issue because #11601 you are passing disabled directly from that which doesn't allow for the Tooltip workaround on the disabled elements here

We need to be able to use the Tooltip to explain to our users why an item is disabled. See example below:

Edit Material demo

@sherodtaylor sherodtaylor changed the title [Autocomplete] added new component called ListOptionComponent [Autocomplete] added new prop called ListOptionComponent Jan 14, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 14, 2020

Details of bundle changes.

Comparing: c5e1ae6...725cfc0

bundle Size Change Size Gzip Change Gzip
@material-ui/lab ▲ +152 B (+0.08% ) 184 kB ▲ +32 B (+0.06% ) 55.1 kB
Autocomplete ▲ +152 B (+0.12% ) 131 kB ▲ +18 B (+0.04% ) 41.2 kB
useAutocomplete ▲ +11 B (+0.08% ) 14.5 kB ▲ +2 B (+0.04% ) 5.24 kB
@material-ui/core -- 361 kB -- 98.7 kB
@material-ui/core[umd] -- 317 kB -- 91.9 kB
@material-ui/styles -- 51.2 kB -- 15.4 kB
@material-ui/system -- 14.8 kB -- 4.07 kB
Alert -- 83.9 kB -- 26.3 kB
AlertTitle -- 64.3 kB -- 20.2 kB
AppBar -- 64.1 kB -- 20.1 kB
Avatar -- 65.4 kB -- 20.6 kB
AvatarGroup -- 62.4 kB -- 19.6 kB
Backdrop -- 68 kB -- 21 kB
Badge -- 65.4 kB -- 20.4 kB
BottomNavigation -- 62.5 kB -- 19.6 kB
BottomNavigationAction -- 75.7 kB -- 23.9 kB
Box -- 71 kB -- 21.7 kB
Breadcrumbs -- 68.1 kB -- 21.4 kB
Button -- 79.9 kB -- 24.5 kB
ButtonBase -- 74.2 kB -- 23.3 kB
ButtonGroup -- 83.4 kB -- 25.5 kB
Card -- 63 kB -- 19.7 kB
CardActionArea -- 75.3 kB -- 23.7 kB
CardActions -- 62.2 kB -- 19.5 kB
CardContent -- 62.1 kB -- 19.4 kB
CardHeader -- 65.2 kB -- 20.5 kB
CardMedia -- 62.5 kB -- 19.7 kB
Checkbox -- 83.2 kB -- 26.3 kB
Chip -- 82.8 kB -- 25.4 kB
CircularProgress -- 64.2 kB -- 20.3 kB
ClickAwayListener -- 3.91 kB -- 1.55 kB
Collapse -- 68.2 kB -- 21.1 kB
colorManipulator -- 3.88 kB -- 1.52 kB
Container -- 63.4 kB -- 19.8 kB
CssBaseline -- 57.7 kB -- 18.1 kB
Dialog -- 83.2 kB -- 25.9 kB
DialogActions -- 62.2 kB -- 19.5 kB
DialogContent -- 62.3 kB -- 19.5 kB
DialogContentText -- 64.2 kB -- 20.2 kB
DialogTitle -- 64.4 kB -- 20.2 kB
Divider -- 62.7 kB -- 19.7 kB
docs.landing -- 51 kB -- 13.5 kB
docs.main -- 617 kB -- 196 kB
Drawer -- 85 kB -- 25.8 kB
ExpansionPanel -- 72.5 kB -- 22.7 kB
ExpansionPanelActions -- 62.2 kB -- 19.5 kB
ExpansionPanelDetails -- 62.1 kB -- 19.4 kB
ExpansionPanelSummary -- 78.3 kB -- 24.7 kB
Fab -- 77 kB -- 24 kB
Fade -- 23.6 kB -- 8 kB
FilledInput -- 73.7 kB -- 22.9 kB
FormControl -- 64.5 kB -- 20.1 kB
FormControlLabel -- 65.6 kB -- 20.6 kB
FormGroup -- 62.1 kB -- 19.5 kB
FormHelperText -- 63.5 kB -- 20 kB
FormLabel -- 63.6 kB -- 19.7 kB
Grid -- 65.2 kB -- 20.4 kB
GridList -- 62.6 kB -- 19.7 kB
GridListTile -- 63.9 kB -- 20 kB
GridListTileBar -- 63.3 kB -- 19.9 kB
Grow -- 24.2 kB -- 8.21 kB
Hidden -- 66.1 kB -- 20.7 kB
Icon -- 62.9 kB -- 19.7 kB
IconButton -- 76.3 kB -- 23.8 kB
Input -- 72.6 kB -- 22.7 kB
InputAdornment -- 65.2 kB -- 20.5 kB
InputBase -- 70.8 kB -- 22.2 kB
InputLabel -- 65.5 kB -- 20.2 kB
LinearProgress -- 65.4 kB -- 20.5 kB
Link -- 66.7 kB -- 21.1 kB
List -- 62.5 kB -- 19.5 kB
ListItem -- 77.3 kB -- 24.2 kB
ListItemAvatar -- 62.2 kB -- 19.5 kB
ListItemIcon -- 62.3 kB -- 19.5 kB
ListItemSecondaryAction -- 62.1 kB -- 19.5 kB
ListItemText -- 65.1 kB -- 20.4 kB
ListSubheader -- 62.9 kB -- 19.8 kB
Menu -- 88.9 kB -- 27.4 kB
MenuItem -- 78.4 kB -- 24.5 kB
MenuList -- 66.2 kB -- 20.7 kB
MobileStepper -- 68 kB -- 21.3 kB
Modal -- 14.5 kB -- 5.05 kB
NativeSelect -- 77 kB -- 24.3 kB
NoSsr -- 2.19 kB -- 1.04 kB
OutlinedInput -- 74.1 kB -- 23.1 kB
Paper -- 62.5 kB -- 19.5 kB
Popover -- 83.3 kB -- 25.8 kB
Popper -- 28.8 kB -- 10.3 kB
Portal -- 2.92 kB -- 1.3 kB
Radio -- 84.3 kB -- 26.6 kB
RadioGroup -- 64.6 kB -- 20 kB
Rating -- 70.6 kB -- 22.7 kB
RootRef -- 4.24 kB -- 1.64 kB
Select -- 116 kB -- 34.4 kB
Skeleton -- 63 kB -- 19.9 kB
Slide -- 25.6 kB -- 8.73 kB
Slider -- 76.8 kB -- 24.3 kB
Snackbar -- 75.5 kB -- 23.6 kB
SnackbarContent -- 63.7 kB -- 20.1 kB
SpeedDial -- 86.4 kB -- 27.2 kB
SpeedDialAction -- 118 kB -- 37.5 kB
SpeedDialIcon -- 64.7 kB -- 20.3 kB
Step -- 62.8 kB -- 19.7 kB
StepButton -- 82.5 kB -- 26.1 kB
StepConnector -- 62.8 kB -- 19.8 kB
StepContent -- 69.3 kB -- 21.7 kB
StepIcon -- 64.8 kB -- 20.2 kB
StepLabel -- 68.7 kB -- 21.7 kB
Stepper -- 65 kB -- 20.5 kB
styles/createMuiTheme -- 16.6 kB -- 5.83 kB
SvgIcon -- 63.2 kB -- 19.8 kB
SwipeableDrawer -- 92.4 kB -- 28.9 kB
Switch -- 82.4 kB -- 26 kB
Tab -- 76.5 kB -- 24.2 kB
Table -- 62.7 kB -- 19.7 kB
TableBody -- 62.2 kB -- 19.5 kB
TableCell -- 64.2 kB -- 20.2 kB
TableContainer -- 62.1 kB -- 19.4 kB
TableFooter -- 62.2 kB -- 19.5 kB
TableHead -- 62.2 kB -- 19.5 kB
TablePagination -- 143 kB -- 41.8 kB
TableRow -- 62.6 kB -- 19.6 kB
TableSortLabel -- 77.6 kB -- 24.4 kB
Tabs -- 85.8 kB -- 27.2 kB
TextareaAutosize -- 5.12 kB -- 2.14 kB
TextField -- 125 kB -- 36.5 kB
ToggleButton -- 76.3 kB -- 24.2 kB
ToggleButtonGroup -- 63.3 kB -- 19.9 kB
Toolbar -- 62.5 kB -- 19.6 kB
Tooltip -- 102 kB -- 32.3 kB
TreeItem -- 74.1 kB -- 23.5 kB
TreeView -- 66.8 kB -- 21 kB
Typography -- 63.8 kB -- 20 kB
useMediaQuery -- 2.51 kB -- 1.06 kB
Zoom -- 23.6 kB -- 8.12 kB

Generated by 🚫 dangerJS against 725cfc0

@oliviertassinari
Copy link
Member

@sherodtaylor Thanks for the proposal. From what I understand, you can solve the problem with the existing API, there is no need to duplicate it.

@oliviertassinari oliviertassinari added the component: autocomplete This is the name of the generic UI component, not the React module! label Jan 14, 2020
@sherodtaylor
Copy link
Author

sherodtaylor commented Jan 14, 2020

@oliviertassinari Can you give me a solution instead of just closing the pr? I've created a demo to show you that the existing api doesn't work ^. Also, it's not duplicating it? What it is doing is allowing control of the actual li not just the children of the li because it's being disabled at that level.

@sherodtaylor
Copy link
Author

@oliviertassinari The current API doesn't support this? See the code sandbox: Edit Material demo

I could rework it to use renderOption but that would be a breaking change to the API. I opted to use this since you have a way to change the ListBoxComponent = 'ul' you should also be able to change the li item using the new API prop called ListOptionComponet = 'li'.

@oliviertassinari
Copy link
Member

@sherodtaylor You would need to customize the CSS of the li element and render the right children.

@oliviertassinari
Copy link
Member

From what I understand, supporting this new prop would make the disabled tooltip use case easier for you to implement.
However, I believe it's an edge case we shouldn't (yet) complexify (by opening more room for mistakes) the API for.

@sherodtaylor
Copy link
Author

@oliviertassinari Can you just reopen the PR because you haven't given me a solution. Look at the implementation I can't pass any props to the 'li' because the only rendering is happing in the children. I think it makes sense for the api change as you are able to change the ListBoxComponent from 'ul' why not be able to control the li with ListOptionComponent. It's not an edge case its common UX to have a tooltip inside of a disabled item.

@sherodtaylor
Copy link
Author

sherodtaylor commented Jan 15, 2020

Also it's not just for tooltip. If a user wants to modify the ListBoxComponent = 'ul' most likely they need to modify the li which this prop allows you to do via ListOptionComponent = 'li'. I don't see how this is a complex API change. It's a prop that was missed in the initial implementation IMO.

I'm happy to keep this closed if you have a solution. Can you fork my code sandbox and take a look please? @oliviertassinari

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 15, 2020

Look at the implementation I can't pass any props to the 'li' because the only rendering is happing in the children.

@sherodtaylor I believe the key factor is the CSS applied, the DOM structure is secondary. Worth case, you can leverage the useAutocomplete hook to implement it.

It's not an edge case its common UX to have a tooltip inside of a disabled item.

This is very possible. I don't know. JedWatson/react-select#745 makes me believe it's an edge case (no mention to the disabled option problem).

I think that if you could open an issue it would be great! As we get upvotes & interactions on the issue, we will consider a different answer. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants