[DateRangePicker] Properly call renderDayProp
#1953
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/5qgcrakgx |
|
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 |
| open | ||
| renderInput={defaultRangeRenderInput} | ||
| onChange={jest.fn()} | ||
| renderDay={day => <div key={String(day)} data-mui-test="renderDayCalled" />} |
There was a problem hiding this comment.
We try to use the default test id attribute name when possible.
| renderDay={day => <div key={String(day)} data-mui-test="renderDayCalled" />} | |
| renderDay={day => <div key={String(day)} data-testid="renderDayCalled" />} |
cc @eps1lon Shouldn't we replace all usage of data-mui-test for data-testid in the core components too?
There was a problem hiding this comment.
I had that in mind yes. I'd prefer to keep them out of the core components though. This way people aren't confused why they aren't available in prod builds.
There was a problem hiding this comment.
I'm not sure we will be able to remove all the data test attributes from the source of the components (only using them in the test). Cases where it seems to be our only option:
- Icons: they are aria-hidden.
- Modal backdrop: no labels, no roles.
| @@ -39,6 +41,7 @@ export const DateRangePickerViewMobile: React.FC<DesktopDateRangeCalendarProps> | |||
| rightArrowButtonProps, | |||
| rightArrowButtonText, | |||
| rightArrowIcon, | |||
| renderDay = (_, props) => <DateRangeDay {...props} />, | |||
There was a problem hiding this comment.
It looks like we will need to refactor the render APIs. I think that the correct API should be:
renderDay = (props) => <DateRangeDay {...props} />,As far as I have seen this pattern used in the community, it gets one argument, with the equivalent of the props.
I will open a RFC for that breaking change
There was a problem hiding this comment.
I strongly believe that we need exactly 2 arguments here. And that's why:
renderDay={(day, selectedDate, DayComponentProps) => {
const date = makeJSDateObject(day); // skip this step, it is required to support date libs
const isSelected =
DayComponentProps.inCurrentMonth && highlightedDays.includes(date.getDate());
return (
<Badge
key={date.toString()}
overlap="circle"
badgeContent={isSelected ? '🌚' : undefined}
>
<Day {...DayComponentProps} />
</Badge>
);
}}Actually an example of rendering custom day will require access to the day that is rendering. I cannot imagine an example where its not. (maybe only if user need to pass custom style).
If we will pass the date as a prop it will need to spend more time to understand how to access the current date. And also will require some kind of notation
(DayProps) => <Day {...DayProps} className={getClassByDay(DayProps.day} /> ({ day, ...other }) => <Day day={day} {...other} className={getClassByDay(day} /> I think this notation is the best
(day, DayProps) => <Day {...DayProps} className={getClassByDay(day} /> There was a problem hiding this comment.
P.S.
selectedDaysmust be removed #1911
There was a problem hiding this comment.
I don't understand why day and selectedDate should come from here. It feels that:
dayshould already be available as part ofDayComponentProps: https://github.com/mui-org/material-ui-pickers/blob/8f895d5e438dcc8f7f04837763979a3009d777dc/lib/src/views/Calendar/Day.tsx#L80.selectedDateshould already be available in the global scope: https://github.com/mui-org/material-ui-pickers/blob/8f895d5e438dcc8f7f04837763979a3009d777dc/docs/pages/demo/datepicker/ServerRequest.example.jsx#L11
There was a problem hiding this comment.
I have described in my previous answer. Because accessing renderDay without using day is mostly useless.
So if we know that day is always required we can simplify accessing it from DayProps.day to the first argument. (react-day-picker's renderDay as reference. )
There was a problem hiding this comment.
I have looked at the codebase of the Autocomplete, we do have cases like this where we decide to inject extra arguments: mui/material-ui#19254 (comment). I wonder if it's the best approach. It feels like we could have a single argument, that it would be simpler.
I really like the simplicity of this variant:
(PickersDayProps) =>
<Day
{...PickersDayProps}
className={clsx(PickersDayProps.className, getClassByDay(DayProps.day})
/> We would need to update Autocompelte too but, It can feel simpler, to have a single place to look for available options. I also wonder if this is not a case simple enough for accepting a Component 🤷♂️. I think that we should really come up with a simple rule to decide with this tradeoff.
There was a problem hiding this comment.
It makes users go to the docs/console.log the props to understand what is passing to the props. When there is only one argument – it is easier.
There was a problem hiding this comment.
It makes users go to the docs/console.log the props to understand what is passing to the props
Why? We can document the props of the Day component, and link this same page.
There was a problem hiding this comment.
Yes, we can, and it is documented. But why developer should look and google for props documentation if he need to just render the same day as we do but do some custom things based on the day of month?
I mean it is not so clear as just simple passing of both day and props.
There was a problem hiding this comment.
The idea would be that we link the PickersDay component page, right in the DateRangePicker prop page. Making the information accessible with one click. We could also have a demo with the pattern.
I think that one advantage is about not having to wonder about when and which arguments should be exposed. A second advantage is that it's closer to how components work.
If we don't go down this route. I would recommend that we force the render props to never have more than 4 arguments, never.
Closes #1856