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

[DatePicker] Reduce coupling of parsing picker input value and props #24319

Merged
merged 2 commits into from Jan 11, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 8, 2021

We were passing the full props object to parsePickerInputValue when we only ever needed the value. Removed this unnecessary coupling so that it's more apparent when we actually need to run the effect that re-computes this.

This revealed that the reason for disabling react/exhaustive-dependencies was outdated. It only served micro-optimization because, right now, every call site of usePickerState passed a stable valueManager so using it as a dependency was extraneous. However, this might break in the future so we need to be aware of it.

In the end we probably don't need the effect anyway (#24315) but in order to reason about the code I need to trim all the unnecessary parts.

@eps1lon eps1lon added the package: lab Specific to @mui/lab label Jan 8, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 8, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 21f5ad8

@oliviertassinari oliviertassinari added the component: date picker This is the name of the generic UI component, not the React module! label Jan 8, 2021
@oliviertassinari oliviertassinari changed the title [lab] Reduce coupling of parsing picker input value and props [DatePicker] Reduce coupling of parsing picker input value and props Jan 8, 2021
@eps1lon eps1lon changed the title [DatePicker] Reduce coupling of parsing picker input value and props [lab] Reduce coupling of parsing picker input value and props Jan 9, 2021
@eps1lon eps1lon removed the component: date picker This is the name of the generic UI component, not the React module! label Jan 9, 2021
@oliviertassinari oliviertassinari added component: date picker This is the name of the generic UI component, not the React module! and removed package: lab Specific to @mui/lab labels Jan 9, 2021
@eps1lon
Copy link
Member Author

eps1lon commented Jan 10, 2021

@oliviertassinari This affects more than the DatePicker. If you think otherwise, please explain it instead of changing things without comment.

@eps1lon eps1lon added package: lab Specific to @mui/lab and removed component: date picker This is the name of the generic UI component, not the React module! labels Jan 10, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 10, 2021

I believe that so far, we have used the "DatePicker" PR label and component: DatePicker issue label to cover the <DatePicker> module, as well as all the other "pickers" components when nothing more specific existed.

It's similar to how we have used the "Tabs" PR label and component: Tabs issue label when we work on the <TabPanel> component that is in the lab.

The incentive was to reproduce the DX as if the component/family of components was under its own repository, isolated. We link this label in the documentation under:

Capture d’écran 2021-01-10 à 11 48 59

https://next.material-ui.com/components/date-picker/

@eps1lon
Copy link
Member Author

eps1lon commented Jan 10, 2021

module

What's that and why did you use angle brackets? Angle brackets resemble JSX syntax.

as well as all the other "pickers" components when nothing more specific existed.

Then why is it named DatePicker and prefixed with the singular components?

The incentive was to reproduce the DX as if the component/family of components was under its own repository, isolated.

Then we should use a different naming scheme.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 10, 2021

We link this label in the documentation under:

But then we should apply all the picker labels not just DatePicker.

@eps1lon eps1lon added component: date picker This is the name of the generic UI component, not the React module! component: date time picker This is the name of the generic UI component, not the React module! component: date range picker This is the name of the generic UI component, not the React module! component: time picker This is the name of the generic UI component, not the React module! and removed package: lab Specific to @mui/lab labels Jan 10, 2021
@eps1lon eps1lon marked this pull request as ready for review January 10, 2021 14:39
@eps1lon
Copy link
Member Author

eps1lon commented Jan 11, 2021

We link this label in the documentation under:

And then also: We link to issues. So this is not even all that relevant.

@eps1lon eps1lon merged commit 36b2f0b into mui:next Jan 11, 2021
@eps1lon eps1lon deleted the feat/pickers/value-manager-simpl branch January 11, 2021 06:32
@oliviertassinari oliviertassinari added new feature New feature or request and removed component: date range picker This is the name of the generic UI component, not the React module! component: date time picker This is the name of the generic UI component, not the React module! component: time picker This is the name of the generic UI component, not the React module! labels Jan 11, 2021
@oliviertassinari oliviertassinari changed the title [lab] Reduce coupling of parsing picker input value and props [DatePicker] Reduce coupling of parsing picker input value and props Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: date picker This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants