Expose mobile{Keyboard,Default}ViewIcon props from PickerToolbar #1706
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/ex6xx9xsz |
The button should be identified by its functionality and not be its icon (also the pen icon is only one of the two icons used on that button).
…ses #1693) To make the previously hard-coded icons configurable.
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| @@ -95,16 +99,12 @@ const PickerToolbar: React.SFC<PickerToolbarProps> = ({ | |||
| {children} | |||
| <IconButton | |||
There was a problem hiding this comment.
Probably we need to also expose props that passing directly to this component.
MobileKeyboardButtonProps
There was a problem hiding this comment.
Okay. At that point, maybe we should remove mobileKeyboardViewButtonClassName? To me it seems like it's just yet another prop on that component -- or are classNames worth treating explicitly in general?
The onClick={toggleMobileKeyboardView} is probably worth keeping as a separate prop, as it must be implemented by the user of the component.
There was a problem hiding this comment.
Ah well, mobileKeyboardViewButtonClassName is used by all the Toolbar implementations, so let's keep it.
There was a problem hiding this comment.
So you proposed "MobileKeyboardButtonProps". I called it "mobileKeyboardViewButtonClassName" so far. Now I realised that there's already "getMobileKeyboardInputViewButtonText = defaultGetKeyboardInputSwitchingButtonText" in the code. Probably we shouldn't have 4 different terms for the same button.
Currently, the DateTimePicker also shows this button on desktop (because it uses the toolbar in its popover, because it has so many different views). Is that desired and will stay like that (then I'd propose to not have "mobile" in the name of this button), or should the desktop DateTimePicker not have the button (then I can send a separate PR to hide it there).
There was a problem hiding this comment.
@dmtrKovalenko @oliviertassinari Any opinions on whether the view switching button should be shown on desktop at all?
And what's the proper name for this button?
| import ClockIcon from '@material-ui/icons/AccessTime'; | ||
| import PenIcon from '@material-ui/icons/Edit'; |
There was a problem hiding this comment.
For consistency with the two imports above?
| import ClockIcon from '@material-ui/icons/AccessTime'; | |
| import PenIcon from '@material-ui/icons/Edit'; | |
| import AccessTimeIcon from '@material-ui/icons/AccessTime'; | |
| import EditIcon from '@material-ui/icons/Edit'; |
| @@ -35,6 +35,8 @@ export type ToolbarComponentProps< | |||
| timeIcon?: React.ReactNode; | |||
| isLandscape: boolean; | |||
| ampmInClock?: boolean; | |||
| mobileKeyboardViewIcon?: React.ReactNode; | |||
| mobileDefaultViewIcon?: React.ReactNode; | |||
There was a problem hiding this comment.
Probably better name will be
| mobileDefaultViewIcon?: React.ReactNode; | |
| mobilePickerViewIcon?: React.ReactNode; |
There was a problem hiding this comment.
And also if it is a public API it should be documented in jsdoc. I am not sure where we are documenting props for Toolbar but please try just adding here a comment with description
|
@Philipp91 Thanks for the pull request. Unfortunately, we can't accept it here. On-going work was moved to v5 in the main repository: https://next.material-ui.com/components/date-picker/. I have transferred the linked issue to mui/material-ui#23604. |
This PR closes #1693.