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

[pickers] Add option to change order of displayed years #11780

Merged
merged 24 commits into from
Sep 19, 2024

Conversation

thomasmoon
Copy link
Contributor

Here is a new prop for the date picker which can be used to reverse the list of years and handle the keyboard input appropriately.

With this feature a dedicated year picker can display the years starting from newest to oldest which is especially useful when a wide range of years is supported but the most commonly selected options will be from more recent years.

mui-x-date-pickers-yearsReversed-demo

I could possibly write a new test for this, but currently I experience problems unrelated to this PR with the time adaptors when running tests, e.g.:

AssertionError: expected 'Europe/Helsinki' to equal 'system'
      + expected - actual

      -Europe/Helsinki
      +system
      
      at Context.<anonymous> (packages/x-date-pickers/src/DateCalendar/tests/timezone.DateCalendar.test.tsx:45:54)
      at processImmediate (node:internal/timers:471:21)```

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 22, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@mui-bot
Copy link

mui-bot commented Jan 22, 2024

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 22, 2024
@zannager zannager added the component: pickers This is the name of the generic UI component, not the React module! label Jan 22, 2024
@flaviendelangle
Copy link
Member

Hi,

Thanks for your contribution, I will add this PR to our grooming of today to see if that's something we want to support out of the box

@thomasmoon
Copy link
Contributor Author

Hi,

Thanks for your contribution, I will add this PR to our grooming of today to see if that's something we want to support out of the box

Thanks a lot! All feedback is welcome if I should develop it a bit further. I couldn’t figure out a good way to override the keyboard events without baking it in, but I did refactor those a bit at the same time. Should test the rtl theme still.

I added custom aria translations for the year in my own code when it’s just a year picker. Those could be good to incorporate but new text keys are required and I can only provide English, Finnish and Swedish.

@LukasTy
Copy link
Member

LukasTy commented Feb 6, 2024

Hello @thomasmoon, thank you for your contribution! 🙏
We want to first identify if this is the best solution for a particular problem.
Could you share what particular problem you are trying to solve?
What kind of data selection you are trying to optimize?

The thing is that we are considering the adoption of MD3 approach to year and month selection and we need to understand if this change will not be a temporary solution that we might end up using soon in the future. 🙈

@LukasTy LukasTy added the enhancement This is not a bug, nor a new feature label Feb 6, 2024
@thomasmoon
Copy link
Contributor Author

thomasmoon commented Feb 9, 2024

Hi @LukasTy, the MUI project has been so beneficial, it's great to find a way to contribute.

The case we have, is a watercraft registration, where a range of dates are offered from 1800-2024 (current year) or 1800-2025 (next year).

MUI-year-picker-case-newest-first

The possible range is huge, but it's rare that a user would register a boat so old. The most common use case would be in the last few years.

For this reason, it makes sense to start the list from the current year and display the list of years in reverse order (descending vs. ascending).

There is a question of consistency, when days and months are always listed in ascending order, but this option would be used only for a standalone year picker. A screen reader states the year when making selections with the keyboard, so it should be intuitive enough in terms of accessibility. I think this should also be compatible with the MD3 approach if it's possible to limit the views to only the year and hide the day and month selection.

In this case we store the value as an integer and have used a numeric field for the input, so the years can also be stepped using the keyboard when the field is selected. Of course the year can also be typed directly.

Until now, we have used a select with the same list of years in descending order, but replacing it with the same component used for other dates will be an improvement in terms of consistency.

Hopefully this case sheds lights on the need for this feature and happy to hear feedback about the approach.

I'll try to fix the few visual regression tests that failed, not sure if they are related, but this is my first time developing the mui-x project so there is a bit of a learning curve.

Thanks for your consideration on this feature and have a nice weekend!

@flaviendelangle
Copy link
Member

flaviendelangle commented Feb 9, 2024

For your specific use case where you only have a year to pick, I'm wondering if an autocomplete wouldn't be better.
The UX of the year view is not great (I would love to rework it) and when use alone without a day view, I don't see a lot of value compared to a flat list.

@thomasmoon
Copy link
Contributor Author

thomasmoon commented Feb 13, 2024

For your specific use case where you only have a year to pick, I'm wondering if an autocomplete wouldn't be better. The UX of the year view is not great (I would love to rework it) and when use alone without a day view, I don't see a lot of value compared to a flat list.

The idea was to use the same recognizable element for all date related inputs. 📅

With the keyboard inputs and aria texts baked in, I think it's a good component for year selection. :accessibility:

@flaviendelangle
Copy link
Member

OK, consistency in the UI is a very good point here 👍

@diegovfeder
Copy link

Hi there,

This yearsReversed prop/feature you've implement is amazing. It's a perfect fit for my project where recent years are more commonly needed - awesome work!

I'm wondering, is there a timeline for when this might be merged or released?
I'd be happy to help move this forward, so if there's anything I can do, please let me know.

Also, if that's not the case, could you share any workarounds to achieve reversed years without altering the base component?

Thanks for your great work on this!

@LukasTy LukasTy changed the base branch from next to master May 3, 2024 15:18
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.

Sorry for the long delay @thomasmoon and, once again, thank you for your contribution!

I gave it more though and I don't feel that this prop would hurt in any way.
If we change to this M3 approach for displaying years, this solution would still remain valid. 🤔

WDYT @flaviendelangle, do you see any other potential pitfalls?

As for the feature itself, it would also be great to also have at least a few tests, especially for the keyboard navigation part.

@thomasmoon are you willing to continue work on it or do you want us to finalize the feature? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This was left in the codebase by mistake.
I've removed it together with a few other files, which were still present in the same folder on this branch.

@LukasTy LukasTy changed the title [pickers] Year picker – yearsReversed option [pickers] Add option to reverse displayed years May 6, 2024
@flaviendelangle
Copy link
Member

WDYT @flaviendelangle, do you see any other potential pitfalls?

For me the only problem is the increased API surface which adds a little bit of complexity in the codebase and a little bit of overhead when reading the doc.
So we must think that this feature is useful enough to justify those costs (which are small costs)

@LukasTy
Copy link
Member

LukasTy commented May 10, 2024

For me the only problem is the increased API surface which adds a little bit of complexity in the codebase and a little bit of overhead when reading the doc.
So we must think that this feature is useful enough to justify those costs (which are small costs)

Well, another option is to improve the customization capabilities in the DateCalendar by adding more slots into DateCalendar for each view or changing viewRenderers such that it would be more trivial to change just the relevant portion of view behavior.
That is arguably more difficult to do right.
Besides, let's look at the yearsPerRow prop, which is probably rarely used. 🙈 😆

@flaviendelangle
Copy link
Member

That is arguably more difficult to do right.

Yes, it would probably increase a lot more the complexity of the codebase

Besides, let's look at the yearsPerRow prop, which is probably rarely used.

This one is mostly here to change the value between mobile and destkop in our components, not for users direct usage

@thomasmoon
Copy link
Contributor Author

Hi guys, thanks again for considering this case and helping to refine it.

I'm definitely willing to help make any needed finalizations and we are still hoping to use the year picker for our case although haven't implemented the forked version yet.

I need to get to know these tests better and push an update and then let's check the situation on how this can be merged.

@LukasTy
Copy link
Member

LukasTy commented May 31, 2024

I'm definitely willing to help make any needed finalizations and we are still hoping to use the year picker for our case although haven't implemented the forked version yet.

I'm glad to hear it, thank you. 👌

I need to get to know these tests better and push an update and then let's check the situation on how this can be merged.

Let us know if you need help with writing tests. Our setup isn't the most conventional one. 🤔

Copy link

github-actions bot commented Jun 6, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 6, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 29, 2024
Copy link

github-actions bot commented Sep 9, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 9, 2024
@thomasmoon
Copy link
Contributor Author

thomasmoon commented Sep 9, 2024

Hi @LukasTy & @flaviendelangle,

I think the updates are set now, feedback is integrated and some tests are added to check the year order.

I also corrected a small oversight with the props.maxDate on the DesktopDatePicker next month test. It was incorrectly props.minDate on one of the tests and I updates the texts a bit for clarity.

Did I make a mistake rebasing the branch? I should have maybe synced the fork first in Github before trying to rebase against the next branch? I was able to integrate some new updates (isRtl) but seems like there are a lot of commits in this PR now. The conflicts that are now showing GitHub don't appear for me locally for some reason.

All tests have passed in the pipeline view. Previously the Argos check showed only two new images related to my updates, but after the rebase there are a lot. Here are direct links to mine that can be found by searching "reverseYears" but perhaps you can advise on how to proceed, I can make a fresh PR if it's easier:
https://app.argos-ci.com/mui/mui-x/builds/23894/107858632
https://app.argos-ci.com/mui/mui-x/builds/23894/107858638

Thanks for your patience, had to put this on hold for the summer holidays.

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 continuing to find room for improvement. 🙏 👍
There are a few small issues with the latest changes, could we address those before merging? 🤔

thomasmoon and others added 6 commits September 17, 2024 08:10
Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com>
Signed-off-by: Thomas Moon <thomas.moon@teamit.fi>
Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com>
Signed-off-by: Thomas Moon <thomas.moon@teamit.fi>
Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com>
Signed-off-by: Thomas Moon <thomas.moon@teamit.fi>
Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com>
Signed-off-by: Thomas Moon <thomas.moon@teamit.fi>
…onent

Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com>
Signed-off-by: Thomas Moon <thomas.moon@teamit.fi>
…verrides for yearsPerRow prop to mobile and static pickers
@thomasmoon
Copy link
Contributor Author

Hi @LukasTy, I packaged up a local version of my fork to take this into use before it hits the official MUI code but I ran into the following type mismatch:

Construct signature return types 'AdapterLuxon' and 'MuiPickersAdapter<never, string>' are incompatible.

Everything seems to actually work ok, although typescript also complains about minDate / maxDate. I think it's related to the adaptor problem. I found that others had encountered the same problem (#11853) and I understand it's due to Luxon settings? Is there any workaround or should v. 6 of the datepickers be used with that?

Thanks again!

@LukasTy
Copy link
Member

LukasTy commented Sep 19, 2024

Hi LukasTy, I packaged up a local version of my fork to take this into use before it hits the official MUI code but I ran into the following type mismatch:

Construct signature return types 'AdapterLuxon' and 'MuiPickersAdapter<never, string>' are incompatible.

Everything seems to actually work ok, although typescript also complains about minDate / maxDate. I think it's related to the adaptor problem. I found that others had encountered the same problem (#11853) and I understand it's due to Luxon settings? Is there any workaround or should v. 6 of the datepickers be used with that?

Hi, you might be experiencing the same problem as we do in our monorepo, where having multiple imports of different adapters throws off TS and it can no longer resolve the TDate properly.
You should be fine if your app uses only a single adapter. 🤔

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.

Awesome work! 🙏 🙌
Thank you for listening to so much feedback and adjusting the PR to perfection. 👍

I found a final small nitpick, but it should be solvable with suggestions via GH. 🤞

Signed-off-by: Lukas Tyla <llukas.tyla@gmail.com>
@LukasTy LukasTy changed the title [pickers] Add option to reverse displayed years [pickers] Add option to change order of displayed years Sep 19, 2024
@LukasTy LukasTy merged commit 5359c70 into mui:master Sep 19, 2024
18 checks passed
@thomasmoon
Copy link
Contributor Author

Awesome work! 🙏 🙌 Thank you for listening to so much feedback and adjusting the PR to perfection. 👍

I found a final small nitpick, but it should be solvable with suggestions via GH. 🤞

Thanks to you and the MUI team! It's a great component library so it's nice to be able to contribute and of course one must listen to all the signals around. 💯

@thomasmoon
Copy link
Contributor Author

Hi LukasTy, I packaged up a local version of my fork to take this into use before it hits the official MUI code but I ran into the following type mismatch:
Construct signature return types 'AdapterLuxon' and 'MuiPickersAdapter<never, string>' are incompatible.
Everything seems to actually work ok, although typescript also complains about minDate / maxDate. I think it's related to the adaptor problem. I found that others had encountered the same problem (#11853) and I understand it's due to Luxon settings? Is there any workaround or should v. 6 of the datepickers be used with that?

Hi, you might be experiencing the same problem as we do in our monorepo, where having multiple imports of different adapters throws off TS and it can no longer resolve the TDate properly. You should be fine if your app uses only a single adapter. 🤔

I think only the one adaptor is in use. Ignoring the TypeScript errors everything does work so I'll just keep my eye on this space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants