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] Rename prop date into parsedValue when it can contain a range #4736

Merged
merged 3 commits into from
May 6, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented May 2, 2022

Goal

Improve the consistency of the naming.
A lot of our inner components received a date prop even when using the DateRangePicker
Things like date: DateRange<TDate> (or more suble, date: TValue which can be a range).

Why parsedValue ?

We already have a props.value which is drilled done even if the typing does not say so

// Note that we are passing down all the value without spread.
// It saves us >1kb gzip and make any prop available automatically on any level down.

This trade-off could be re-evaluated at some point

Work

  • Rename the pickerProps.date of usePickerState into pickerProps.parsedValue
  • Use parsedValue in all the components that are being given the pickerProps (CalendarOrClockPicker and DateRangePickerView)
  • Write the changelog for the breaking change

Questions

Should we rename date into parsedValue in deeper views ? Like CalendarPicker or ClockPicker
When reaching those components, we know that it is not a range so the date naming is not incoherent. With that being said, maybe we should aim for consistency.

What's next ?

In DayPicker I kept the date name even if it can be given a range.
That's because I want to do deeper changes to this component to fix #4703.

@flaviendelangle flaviendelangle self-assigned this May 2, 2022
@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label May 2, 2022
@mui-bot
Copy link

mui-bot commented May 2, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 251.7 563.3 457.5 413.86 115.631
Sort 100k rows ms 446.2 1,023.4 659.6 723.32 187.628
Select 100k rows ms 136.4 399.2 192.8 223.72 90.623
Deselect 100k rows ms 109.7 212.2 151.4 155.06 32.782

Generated by 🚫 dangerJS against d389bc4

@alexfauquette
Copy link
Member

Should we rename date into parsedValue in deeper views?

For me it makes sense to have two namings, one for values which can either be a range of date or single date. And another naming for when we know what type it is

@flaviendelangle
Copy link
Member Author

There is a small nuance, the two criteria is "Components in which the value can be a range OR called with a prop spread from a hook (often usePickerState) on which the value can be a range".

And this nuance is ... not great 😆

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 5, 2022
@github-actions
Copy link

github-actions bot commented May 5, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 5, 2022
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

It took me some time to understand the nuance. The new naming does not cover the minimal set of variables because of the hook, but at least it covers all the needed ones:

  • I see parsedValue, maybe it can be date or range, maybe it is only a date. If I have a doubt I take care of handling both
  • I see date I know it is only a date, I don't need to take care

@flaviendelangle flaviendelangle merged commit 20ce3ac into mui:master May 6, 2022
@flaviendelangle flaviendelangle deleted the rename-prop-date branch May 6, 2022 11:05
@flaviendelangle flaviendelangle mentioned this pull request May 6, 2022
2 tasks
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Add support for @js-joda/core
3 participants