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

Add prop to render elements after calendar (#1795) #1945

Closed

Conversation

Jenselme
Copy link

@Jenselme Jenselme commented Jun 29, 2020

This allows us to render anything after the calendar (buttons, text…)
by passing the renderCalendarInfo props to DateRangePicker.

Right now this PR is missing tests and documentation for the new props.

Before doing that, I'd like some inputs from you to see whether my approach seems correct to you. I'd also like to update the DatePicker to provide similar functionnality if it is. I tried to follow the ideas discussed in the related issue #1795

My interogation is: I am rendering the extra information at the right place (after the Calendar component) or should I render it inside the Calendar component.

@vercel
Copy link

vercel bot commented Jun 29, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/qa7ciashm
✅ Preview: https://material-ui-pi-git-fork-bureauxlocaux-feat-enable-button-8e8124.mui-org.vercel.app

@cypress
Copy link

cypress bot commented Jun 29, 2020



Test summary

78 0 3 0


Run details

Project material-ui-pickers
Status Passed
Commit 3407770
Started Sep 8, 2020 3:21 PM
Ended Sep 8, 2020 3:23 PM
Duration 01:47 💡
OS Linux Debian - 10.0
Browser Chrome 80

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

@@ -25,6 +25,7 @@ export interface ExportedDesktopDateRangeCalendarProps {
* @default 2
*/
calendars?: 1 | 2 | 3;
renderCalendarInfo?: React.ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

Will be better to have a function that will take the currentMonth as an argument.

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI. discussion labels Jun 29, 2020
@oliviertassinari
Copy link
Member

This concern is quite interesting. I think that we can use it as a case study, also with #1907 around how we can improve the customization API. I would propose we keep this pull request on hold until we resolve mui/material-ui#21453.

I went a bit looking into react-day-picker and found a components API https://react-day-picker-next.netlify.app/docs/custom-components. It looks heavily inspired by https://react-select.com/components.

@Jenselme
Copy link
Author

I went a bit looking into react-day-picker and found a components API https://react-day-picker-next.netlify.app/docs/custom-components. It looks heavily inspired by https://react-select.com/components.

Do you think the components API is more the way to go than the function?

I haven't seen mui/material-ui#21453 before creating this but yes, a uniform API would be better. Let me know if I can help. Currently, I'll have to use a custom build of the project to move forward with my project, but I'd like to see this go upstream as fast as possible. Perhaps we can also move forward with the function right now and change it in a new version (if I understand correctly version 4 is still in alpha so we can do this).

@oliviertassinari
Copy link
Member

Perhaps we can also move forward with the function right now and change it in a new version (if I understand correctly version 4 is still in alpha so we can do this).

@Jenselme Sorry, this is not possible, we would only merge changes once we are confident we have explored enough the alternative options.

@dmtrKovalenko
Copy link
Member

In my opinion render function here will be the best choice. Because it will be possible to render it dynamically for months without having to introduce new state.

My idea:

<DatePicker 
   renderCalendarInfo={(month) => <div> You will not be able to do something before {moth.sub({ month: 1 })} </div>
/>

Moreover we need to handle it for DateRangePicker where it is extremely useful. Something like

<DateRangePicker 
   renderLastCalendarInfo={(month) => <Button> I do not need return ticket </Button>} 
/>

It is the most required functionality I suppose

@Jenselme
Copy link
Author

Jenselme commented Jul 1, 2020

@dmtrKovalenko so with two functions, one to allow rendering before the calendar and one to allow rendering after?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 1, 2020

What about this approach? This configuration can reduce the need for new custom APIs, we can reuse the existing component separation of the internal, progressively expose more to the public:

import { DateRangePicker, Calendar } from '@material-ui/pickers';

function MyCalendar(props) {
  return <Calendar classes={{}} {...props} />;
}

return (
  <DateRangePicker components={{ Calendar: MyCalendar }} />
);

At this point, you can render and customize the component as you see fit. Maybe even leverage the classes API: #578.

@dmtrKovalenko
Copy link
Member

I am not sure, will it be "over" customized?
There is the point when customization ability becomes a pain. Here as for me render function will fit the best

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 3, 2020

@dmtrKovalenko I think that in an ideal world, we would expose an API that allows composition, the closer we are to the DOM notes, the better.

<DatePicker>
  <DatePickerInput />
  <DatePickerPopup>
    <Calendar />
  </DatePickerPopup>
<DatePicker>

Now, in practice, this is likely overkill, it requires a lot of coordination.

I think that we can benefit from your existing reflection on what's the best wait to separate the different components (the current private components) and simply reexpose them. I have added a couple of more thoughts on the problem on mui/material-ui#21453 (comment)

@oliviertassinari
Copy link
Member

I am not sure, will it be "over" customized?
There is the point when customization ability becomes a pain. Here as for me render function will fit the best

I think that over-customizability is a concern on its own and that can be resolved independently. No matter the direction we go in.

@Jenselme
Copy link
Author

@oliviertassinari @dmtrKovalenko did you decide for a path forward? I'd really like to complete this and see it merged in September ;-)

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 31, 2020

@Jenselme Yes, we would be going with mui/material-ui#21453 (comment). However, we might still change the names of these props. slotComponents could become components and slotProps -> componentsProps.

@Jenselme
Copy link
Author

@oliviertassinari any idea when this will be stable?

@oliviertassinari
Copy link
Member

@Jenselme What would be stable? The name? The name can be changed later on, it's a low-cost change, no need to optimize for it.

@Jenselme
Copy link
Author

The name can be changed later on, it's a low-cost change, no need to optimize for it.

If it's just the names, I can update this PR yes.

@oliviertassinari
Copy link
Member

@Jenselme Also, please mind this comment: #2092 (comment).

@Jenselme
Copy link
Author

Jenselme commented Sep 7, 2020

@oliviertassinari if I understood correctly, it should be something like this: 4b82dd0 Am I right?

@oliviertassinari
Copy link
Member

Yes, it looks close :)

This allows us to render anything after the calendar (buttons, text…)
by passing the renderCalendarInfo props to DateRangePicker.
@Jenselme Jenselme force-pushed the feat/enable-buttons-calendars branch from 4b82dd0 to 05d8a95 Compare September 8, 2020 15:15
@vercel vercel bot temporarily deployed to Preview September 8, 2020 15:15 Inactive
@Jenselme Jenselme marked this pull request as ready for review September 8, 2020 15:15
@Jenselme
Copy link
Author

Jenselme commented Sep 8, 2020

I just updated the PR, that should be it. I'm not used to TS so I may have done mistakes with it. I know I had to add a // @ts-ignore in my example.

For testing, should I use "calendar customization" in docs/pages/regression/Regression.tsx to validate this?

I also noted that this PR won't be merged directly because of mui/material-ui#21453 (comment) But I think the result is reviewable anyway.

@codecov-commenter
Copy link

Codecov Report

Merging #1945 into next will increase coverage by 0.02%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1945      +/-   ##
==========================================
+ Coverage   92.44%   92.47%   +0.02%     
==========================================
  Files          84       84              
  Lines        2330     2338       +8     
  Branches      453      459       +6     
==========================================
+ Hits         2154     2162       +8     
  Misses        135      135              
  Partials       41       41              
Impacted Files Coverage Δ
...src/DateRangePicker/DateRangePickerViewDesktop.tsx 77.77% <83.33%> (+1.77%) ⬆️
.../src/DateRangePicker/DateRangePickerViewMobile.tsx 95.45% <100.00%> (+1.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91dcbaf...3407770. Read the comment docs.

@oliviertassinari
Copy link
Member

@Jenselme Thanks for working on the problem. I'm closing as we have moved the component to https://github.com/mui-org/material-ui.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants