-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[RFC] Proposal for the future DX of the Date and Time Pickers #15613
base: master
Are you sure you want to change the base?
Conversation
0e16795
to
7060489
Compare
7060489
to
014bcfb
Compare
2d68e94
to
0470967
Compare
0470967
to
053cba2
Compare
f6b1766
to
899728e
Compare
dfca292
to
ede2530
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't had a chance to read through all of it yet, but it's looking great.
<RangeCalendar.GoToMonth target="previous">◀</RangeCalendar.GoToMonth> | ||
{visibleMonth.format('MMMM YYYY')} | ||
<RangeCalendar.GoToMonth target="next">▶</RangeCalendar.GoToMonth> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming suggestion:
<RangeCalendar.GoToMonth target="previous">◀</RangeCalendar.GoToMonth> | |
{visibleMonth.format('MMMM YYYY')} | |
<RangeCalendar.GoToMonth target="next">▶</RangeCalendar.GoToMonth> | |
<RangeCalendar.MonthNavigation direction="previous">◀</RangeCalendar.GoToMonth> | |
{visibleMonth.format('MMMM YYYY')} | |
<RangeCalendar.MonthNavigation direction="next">▶</RangeCalendar.GoToMonth> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the name of the component, on the POC I'm currently using Calendar.SetVisibleMonth
.
The idea was that it's consistent with the state it updates (which is named visibleDate
).
Calendar.MonthNavigation
also works well, maybe we can wait to see other similar button components on our advanced components and then try to have a coherent naming strategy (with / without a verb)
For the target
/ direction
prop, I think direction
is problematic now that we can also pass a date directly to the component and not only "next" / "previous".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sort of on the same page about the naming here. Feels a bit off to name a component like you would name a function 🤔 I'm all in for keeping the visibleMonth/Date/Year
in the name, but maybe visibleMonthNavigation
(or so9mething in that area 😅 ) could work better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most important is to be consistent with the other Base UI / Base UI X primitives.
I see that Base UI uses <Menu.Trigger />
and I guess the big question is, if you have two triggers, would you name it <MenuTriggerA />
(trigger is a verb) or <Menu.ATrigger />
(trigger is a noun).
I'm totally fine with both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we have two triggers (or two very similar actions) for the exact same component?
I could imagine having something like SomeComponent.Trigger
and OtherComponent.Trigger
but I might very well be missing stuff
But in the scenario you are describing, I would lean towards <Menu.ATrigger />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was more a way to say that the current nomenclature <Menu.Trigger />
on Base UI can be understood both as a noun and as a verb.
So when we try to create buttons with more detailed names, some of us might prefer to treat it as a noun or as a verb.
<React.Fragment> | ||
<header> | ||
<Calendar.SetVisibleMonth target="previous">◀</Calendar.SetVisibleMonth> | ||
{visibleMonth.format('MMMM YYYY')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{visibleMonth.format('MMMM YYYY')} | |
<Calendar.MonthLabel format="MMMM YYYY" /> |
Wonder if this should be wrapped in a component as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a small typo here, it should be visibleDate
not visibleMonth
that is exposed.
On the 1st version, I had a <Calendar.FormattedValue />
component that was taking a format
prop and formatting the visible date.
The problem was that on some example I wanted to use the visibleDate
, on others I wanted to use the selected value
. So I needed either two components, or one component with a prop to say which state was targetted.
And when you render two months, you may want to show the two month name in the header, so maybe also an "offset" prop.
It was getting quite complex for the very little shortcut it gives.
Which is why I went for not providing a component, but instead rely on direct state access, at least until we have more value to give to this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good 🔥 I'm leaving a few nitpicks about naming. I didn't dive deep into everything - I will go through the other pages in the following days 🙈
|
||
### Without Material UI | ||
|
||
The user can use the `<PickerField.Clear />` component to add a button to clear the value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the naming of this. Maybe:
The user can use the `<PickerField.Clear />` component to add a button to clear the value: | |
The user can use the `<PickerField.ClearButton />` component to add a button to clear the value: |
It's a nitpick, and every component that triggers some sort of action should probably have a consistent naming, so feel free to ignore my comment if it conflicts with what you have planned for other parts of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Base UI team used <Menu.Trigger />
not <Menu.TriggerButton />
, same for <Menu.Increment />
.
So I don't think adding "Button" would fit nicely.
But maybe <PickerFIeld.Clear />
is too vague and short and <PickerField.ClearValue />
would be better (or something else).
Clearly the naming of the buttons seems to be one of the hot spot of the new DX 😆
The new DX is a good opportunity to discuss the behavior of this prop. | ||
The behavior should either be: | ||
|
||
1. `onClear` is defined on `<PickerField.Root />`. It is also fired when doing <kbd class="key">Ctrl</kbd> + <kbd class="key">A</kbd> and then <kbd class="key">Backspace</kbd>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having one onClear
method defined on the higher level, that is executed every time a clearing action is triggered (click on the button or by keyboard). Makes things more consistent IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also what I would prefer thinking about it right now.
<Calendar.DaysGridBody> | ||
{({ weeks }) => | ||
weeks.map((week) => ( | ||
<Calendar.DaysWeekRow value={week}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it slightly confusing that The prefix of most of the component names here is DaysGrid
but this one is called DaysWeekRow
. We could consider renaming it to DaysGridRow
for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting...
I went for DaysWeekRow
because I wanted to highlight that 1 row = 1 week.
But maybe it's so obvious that it's not worth the slight inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe it's so obvious that it's not worth the slight inconsistency.
I think it's clear that week == row. And I'm not sure we would have anything that we could introduce to make it unclear (some other component that would be called row for instance). So I'm all for consistency in this case 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update both the RFC and the POC tomorrow to see if somebody thinks otherwise 😆
And related question, would you name the params in the render function weeks
and days
or rows
and cells
(in which case the month and year render functions would have cells
instead of months
and years
)
I went back and forth on this one.
On the time view I chose option
on the HourOptions
and equivalent primitives, because if I named it with the name of the section, the MeridiemOptions
was receiving meridies
if you follow the latin plural and thats far from obvious 😆 , but I could not find a better alternative.
Some maybe cells
/ rows
is better, and options
for the time since it's also the name of the component (or cells
if I rename the Option
component into Cell
and we have something consistent everywhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm...interesting question 🤔 I don't have a very very strong preference there.
For me cells
/rows
is more something that has to do with the layout. So Calendar.DaysGridRow
and Calendar.DaysCell
make sense as something that clearly helps you build your layout. But the render function params indicate what you are displaying inside the layout so maybe keeping weeks
, days
, months
and years
is somewhat justified too 🤔 Not sure if that makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does make sense
I think it's super subjective, and if you asked a lot of people I wouldn't be surprised if we ended up with a 40/60 answer ratio
<RangeCalendar.GoToMonth target="previous">◀</RangeCalendar.GoToMonth> | ||
{visibleMonth.format('MMMM YYYY')} | ||
<RangeCalendar.GoToMonth target="next">▶</RangeCalendar.GoToMonth> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sort of on the same page about the naming here. Feels a bit off to name a component like you would name a function 🤔 I'm all in for keeping the visibleMonth/Date/Year
in the name, but maybe visibleMonthNavigation
(or so9mething in that area 😅 ) could work better?
Link to the RFC (all the pages are in the expanded page group on the left nav bar).
Preliminary notes
Which components?
Calendar
RangeCalendar
PickerField
Picker
(+ maybe aRangePicker
)DigitalClock
With those 5 / 6 components, we would cover almost all the UI of the date and time pickers.