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

[DatePicker] Display week number #6144

Merged
merged 72 commits into from
Nov 22, 2022
Merged

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Sep 13, 2022

Fix #4606

doc preview

Screenshot 2022-11-22 at 19 08 38

After agreeing on the main pipeline:

  • Test
  • Refine design
  • Update adapter for native support
    • date-fns
    • dayjs
    • luxon
    • moment

@mui-bot
Copy link

mui-bot commented Sep 13, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-6144--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 628 1,018.9 628 793.26 182.988
Sort 100k rows ms 610.3 1,159.8 705.2 858.36 190.458
Select 100k rows ms 221.8 390.5 287.5 287.82 62.387
Deselect 100k rows ms 145.1 275.3 195.9 204.34 43.427

Generated by 🚫 dangerJS against 672cc3b

@flaviendelangle
Copy link
Member

Maybe @gerdadesign review would be interesting on this one

I feel like the week number is to close from the 1st column of days

@alexfauquette
Copy link
Member Author

@flaviendelangle I agree. I just wanted to see before with you and @LukasTy if we agree on the DX.

For now, there is no way to get the week number from date-io. So I propose a two steps strategy:

  • We release a version with a callback and let to developer the responsibility of computing the week number.
  • We extend adapters to support week number

This should avoid breaking changes

@flaviendelangle
Copy link
Member

If we were to add a method in the adapter and release it under a minor version, it would not be breaking right ?

@alexfauquette
Copy link
Member Author

Yes. For developpers using getWeekNumber the only difference will be that they can remove this prop when we add the adapter method.

It's like introducing the customization callback before the default behavior 😅

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

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

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 21, 2022
@github-actions
Copy link

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 21, 2022
@alexfauquette
Copy link
Member Author

I do not understand those typescript errors

error TS2345: Argument of type 'MuiPickersAdapter<TDate>' is not assignable to parameter of type 'MuiPickersAdapter<TDate | null>'.

Yes MuiPickersAdapter<TDate> and MuiPickersAdapter<TDate | null> are incompatible, but I do not find where does this null comes from

@flaviendelangle
Copy link
Member

flaviendelangle commented Sep 21, 2022

CalculateRangeChangeOptions expects newDate: TDate
And you are passing rangePreviewDay which has the type TDate | null in DateRangePickerViewDesktop.
So TS consider that the TDate of CalculateRangeChangeOptions equals TDate | null in DateRangePickerViewDesktop.

Which causes your problem: the utils of type MuiPickersAdapter<TDate> in DateRangePickerViewDesktop is considered to equal MuiPickersAdapter<TDate | null> by CalculateRangeChangeOptions.


This kind of issue often comes from similar problem, the one prop TS uses to infer the generic is nullable when it should not.

@alexfauquette
Copy link
Member Author

Thanks for the explanation. I fixed most of them. The remaining ones are about localization with TInputDate. I'm waiting for the PR removing this type to do the final clean-up

Sounds to be an interesting PR for detecting typing errors

Copy link
Member

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

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

I feel like the week number is to close from the 1st column of days

I agree. It can be solved in UserLand, but a bit more spacing by default can help visually group the more important information on the calendar.

image

Another customization I tried on the sandbox above was to change the calendarWeekText, but I couldn't get it working.

localeText={{ calendarWeekNumberText: "KW" }} Am I missing something?

p.s.: KW stands for Kalenderwoche, the used acronym in Germany.

@flaviendelangle
Copy link
Member

@joserodolfofreitas this translation key is a callback.
It is the one used to render 45 / 46 etc... on your screen, so it needs to by dynamic.
For the header you are looking for calendarWeekNumberHeaderText

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Nov 17, 2022

For the header you are looking for calendarWeekNumberHeaderText

Thanks! It'll probably be covered by any community translation, but should it work like this?

localeText={{ calendarWeekNumberHeaderText: "KW" }} to replace the "#" ?

https://codesandbox.io/s/custom-week-numbers-es327n?file=/demo.tsx

@flaviendelangle
Copy link
Member

flaviendelangle commented Nov 17, 2022

Oh indeed we have a big bug with the nested LocalizationProvider

The following work: https://codesandbox.io/s/custom-week-numbers-forked-2d033y?file=/demo.tsx
But if you put your locale on the LocalizationProvider, it gets lost

EDIT: Fixed by #6895

To shows all days of displayed weeks, included those outside of the current month, use `showDaysOutsideCurrentMonth`.

By default, only weeks of the current month are displayed, but you can provide a total number of week to display with `fixedWeekNumber` prop.
This value is usually set to `6` for Gregorian calendars, because months display can vary between 4 and 6 weeks.

{{"demo": "CustomMonthLayout.js"}}

### Add week number
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Add week number
### Display week number

@@ -98,13 +98,22 @@ You can take advantage of the [PickersDay](/x/api/date-pickers/pickers-day/) com

You can customize the month layout with some props.

### Show additional days

To shows all days of displayed weeks, included those outside of the current month, use `showDaysOutsideCurrentMonth`.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can review the props names to be more consistent between each other (in another PR).
For instance, would it make sense to rename this property to displayDaysOutsideCurrentMonth?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not. Can you create an issue for it?

### Add week number

To display week number, use the `displayWeekNumber`.
You can customize the string displayed, by using the localization key`localeText.calendarWeekNumberText`.
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure if there are use cases where users need to render something different for the week numbers, I'd think there's more chance that they want to update the header.

Suggested change
You can customize the string displayed, by using the localization key`localeText.calendarWeekNumberText`.
You can customize the calendar week header by using the localization key `localeText.calendarWeekNumberHeaderText`.
As well you can also customize what's rendered as a calendar week number, using a callback for the localization key `localeText.calendarWeekNumberText`

Should we also add an example for the callback usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not super inspired by what could be a customized week number, so I simply added a point at the end of week numbers, and I override the header to set the default "#". It does not change any thing but it highlight the localization key.

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

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

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Great work !
Sorry for the delay reviewing that one

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 21, 2022
Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Nice catch !

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

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 22, 2022
@alexfauquette alexfauquette merged commit d5ba576 into mui:next Nov 22, 2022
@alexfauquette alexfauquette deleted the weekNumber branch November 22, 2022 13:35
@oliviertassinari oliviertassinari added the new feature New feature or request label Nov 22, 2022
@Eshakheni123
Copy link

why we can not click weekNumber.
I need functionality like this, if I click weekNumber then should be the render method.
something like this:
https://codesandbox.io/s/react-calendar-show-week-numbers-and-fixed-number-of-weeks-forked-hi4y1t?file=/src/index.js

@alexfauquette
Copy link
Member Author

@Eshakheni123 Could you open an issue such that we can have a distinct discussion about your feature request

Would be nice in your issue to add some context, about what would be the use-case solved by being able to click on week-numbers: For which kind of application will it be used, and what would be the behavior when clicking on it?

@Eshakheni123
Copy link

Thanks for your Response. I have created a new issue.

#7715

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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DatePicker] Display week numbers
9 participants