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

[DateRangePicker] Fix currentMonthCalendarPosition not scrolling to future sibling #14442

Merged

Conversation

GMchris
Copy link
Contributor

@GMchris GMchris commented Sep 1, 2024

fixes #14418

@mui-bot
Copy link

mui-bot commented Sep 1, 2024

Deploy preview: https://deploy-preview-14442--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against d4ade93

@michelengelen michelengelen added bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! labels Sep 2, 2024
@GMchris GMchris force-pushed the date-range-calendar/fix-current-month-scroll branch from c92495a to 4e046d8 Compare September 2, 2024 23:57
@LukasTy LukasTy added plan: Pro Impact at least one Pro user component: DateRangePicker The React component. labels Sep 6, 2024
@LukasTy LukasTy changed the title [date-pickers-pro][DateRangeCalendar] Fix currentMonthCalendarPosition not scrolling to future sibling [DateRangePicker] Fix currentMonthCalendarPosition not scrolling to future sibling Sep 6, 2024
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Thank you for noticing and handling this issue! 🙏

I took the liberty to add a test asserting this behavior.

@GMchris Your changes impact the commercially licensed code. For any changes of this nature, we require contributors to sign the MUI’s Contributor License Agreement (CLA).

However, this change looks like a pretty trivial bugfix that we could accept without the CLA process if we both agree that the fix is trivial: https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0?pvs=4#ede845886e494cb4b802a67563152130.
WDYT?
If you wish to go through the CLA process, please let me know and follow this guide. 😉

@GMchris
Copy link
Contributor Author

GMchris commented Sep 7, 2024

Hey, @LukasTy , I've followed the CLA guide and submitted both form and email.

@oliviertassinari oliviertassinari added the CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 label Sep 7, 2024
@LukasTy LukasTy enabled auto-merge (squash) September 9, 2024 06:48
@LukasTy LukasTy merged commit 06b8c13 into mui:master Sep 9, 2024
16 checks passed
@oliviertassinari
Copy link
Member

@LukasTy Two side thoughts:

  1. When requesting for a CLA, to add the CLA: required label to prevent someone else from merging without noticing. Now, to be fair, the fix is really simple, there isn't much IP to get, so we could have almost merged without.
  2. https://mui.com/x/react-date-pickers/date-range-calendar/#choose-the-months-to-render I have my doubts about this currentMonthCalendarPosition props, I would almost better see a prop that is a date that represents the current month displayed. I mean, we have but shouldn't people be able to use -10, +10 too?

@LukasTy
Copy link
Member

LukasTy commented Sep 9, 2024

Thank you for the insights @oliviertassinari.

  1. When requesting for a CLA, to add the CLA: required label to prevent someone else from merging without noticing. Now, to be fair, the fix is really simple, there isn't much IP to get, so we could have almost merged without.

Thanks, next time I will either not approve the PR or add the necessary label until the CLA topic is resolved.
This is the second time we have had a "trivial"-like change and previously agreed to merge without signing. 😉

2. https://mui.com/x/react-date-pickers/date-range-calendar/#choose-the-months-to-render I have my doubts about this currentMonthCalendarPosition props, I would almost better see a prop that is a date that represents the current month displayed. I mean, we have


but shouldn't people be able to use -10, +10 too?

It can be -10 or +10 if someone needs it, just like anyone is free to specify whatever amount of calendars they want (except for overriding the TS error or PropTypes warning). 😉
We have previously had a currentMonth prop, but that was replaced with referenceDate.
What use case do you see for -10 or +10? 🤔

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 9, 2024

@LukasTy Ah, ok, so referenceDate and currentMonthCalendarPosition are meant to be used at the same time. This makes sense 👍. I could see a warning when using a value below 1 and above 3 to explain the alternative.

The thing in my mind was about: how can a developer control the view displayed in the pickers? react-day-pickers allows people to https://daypicker.dev/docs/navigation#onmonthchange. A month prop sounds too specific though, I would expect a date prop that the component ensures is "visible" in the UI.

referenceDate is close but seems to be only the default value.

@LukasTy
Copy link
Member

LukasTy commented Sep 10, 2024

The thing in my mind was about: how can a developer control the view displayed in the pickers? react-day-pickers allows people to https://daypicker.dev/docs/navigation#onmonthchange. A month prop sounds too specific though, I would expect a date prop that the component ensures is "visible" in the UI.

referenceDate is close but seems to be only the default value.

Great point, thank you for bringing up this possible need. 👍
There is already an issue for it (#10869), I have added specific priority to it so that it does not get lost. 👍
We might have overlooked this need with the referenceDate implementation. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 component: DateRangePicker The React component. component: pickers This is the name of the generic UI component, not the React module! plan: Pro Impact at least one Pro user
Projects
None yet
5 participants