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

[Pickers] Don't pass down unnecessary props #27256

Closed
wants to merge 12 commits into from

Conversation

michal-perlakowski
Copy link
Contributor

@michal-perlakowski michal-perlakowski commented Jul 13, 2021

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 13, 2021

Details of bundle changes (experimental)

@material-ui/lab: parsed: +1.62% , gzip: +0.80%

Generated by 🚫 dangerJS against ffffb42

@michal-perlakowski
Copy link
Contributor Author

I just noticed this comment in CalendarPicker.test.tsx:

// TODO: Fix CalendarPicker is not spreading props on root

So it seems the props should be spread after all. I'll change this PR to spread props on both MonthPicker and YearPicker, but without props that are irrelevant here, like renderInput and inputFormat. @oliviertassinari What about PickersCalendar? It currently returns a Fragment - should we change it to a div?

@eps1lon
Copy link
Member

eps1lon commented Jul 13, 2021

TODO: Fix CalendarPicker is not spreading props on root

Could you check when this was added?

I think I planned to explicitly pick which prop goes where and nost just spread everything down. This may add to the bundle size but makes it easier to track which prop goes where.

@michal-perlakowski
Copy link
Contributor Author

@eps1lon It was added during migration to emotion in #26390.

@oliviertassinari
Copy link
Member

Won't these changes break the conformance tests of this public component?

@michal-perlakowski
Copy link
Contributor Author

michal-perlakowski commented Jul 13, 2021

@oliviertassinari Yes, the current changes broke the conformance test, but we could just disable that check, like in the YearPicker. The question is if we want to:

  1. Pass props to the root component by spreading other props.
  2. Pass props to the root component (and maybe other components) through ComponentsProps.
  3. Not pass custom props to the root component.

I think this should be unified across different picker views.

@eps1lon
Copy link
Member

eps1lon commented Jul 14, 2021

I think the problem is rather the components that use MonthPicker and just pass unsupported props. That's where we want to filter the props.

Removing conformance is a non-starter.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 14, 2021
michal-perlakowski and others added 3 commits July 14, 2021 20:07
# Conflicts:
#	packages/material-ui-lab/src/internal/pickers/Picker/Picker.tsx
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 14, 2021
@michal-perlakowski michal-perlakowski changed the title [DatePicker] Don't spread other props on MonthPickerRoot [Pickers] Don't pass down unnecessary props Jul 14, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 18, 2021
@eps1lon eps1lon changed the base branch from next to master September 14, 2021 09:29
@mnajdova
Copy link
Member

Seems like there is not much activity on the pull request, and considering that we are moving the pickers to https://github.com/mui-org/material-ui-x I am closing the pull request. cc @flaviendelangle

@mnajdova mnajdova closed this Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Custom input component (renderInput) doesn't work with month view
6 participants