-
Notifications
You must be signed in to change notification settings - Fork 833
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
[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 summaryRun 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 |
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.
Thanks for the test case :)
open | ||
renderInput={defaultRangeRenderInput} | ||
onChange={jest.fn()} | ||
renderDay={day => <div key={String(day)} data-mui-test="renderDayCalled" />} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S.
selectedDays
must be removed #1911
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 don't understand why day
and selectedDate
should come from here. It feels that:
day
should already be available as part ofDayComponentProps
: https://github.com/mui-org/material-ui-pickers/blob/8f895d5e438dcc8f7f04837763979a3009d777dc/lib/src/views/Calendar/Day.tsx#L80.selectedDate
should 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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