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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(module:datepicker): weekday header fix #1054

Merged
merged 7 commits into from
Feb 5, 2021

Conversation

anddrzejb
Copy link
Member

馃 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • Bundle size optimization
  • Performance optimization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

馃敆 Related issue link

Fixes #1045

馃挕 Background and solution

The issue itself was simple to fix. Had to just transfer code from DatePickerDatePanlel to DatePickerDateTimePanel, because there was an old evaluation of starting date.
But I dig a bit deeper and I found a possible problem, that I decided I will try to fix.
I am referring to DatePickerLocale.cs. There is a field there ShortWeekDays that is an array of shorthand weekday names. This is exposed and can be set by a consumer. The problem is when a consumer decides to put the order of the weekdays starting from a different index than Sunday. Which is not unexpected - for me, a more natural order is { "Mo", "Tu", "We", "Th", "Fr", "Sa", "Su" }. In such case the order will be screwed up and FirstDayOfWeek will have to point to 'Sunday" for this to actually start showing from 'Monday". So I added a new property MondayIndex. It will store index in ShortWeekDays that will be Monday. So in the example I gave it should be MondayIndex = 0.
I also added a new method in DatePickerLocale.cs so properly ordered array can be fetched (ShortWeekDaysSorted).
Another method I added is SetShortWeekDays(string[] shortWeekDays, int mondayIndex) that will set the properties. To be honest, I would prefer to completely get rid of properties MondayIndex and ShorWeekDays (in favour of private fields) and rely completely on SetShortWeekDays, but I can see it is needed because of json deserialization.

What do you think? If this is ok, I will also try to make changes to locale json files to include the mondayIndex (as I did for 3 languages).

馃摑 Changelog

Language Changelog
馃嚭馃嚫 English New MondayIndex property on DatePickerLocale.cs class that stores Monday index in ShortWeekDays.
馃嚚馃嚦 Chinese

鈽戯笍 Self Check before Merge

鈿狅笍 Please check all items below before review. 鈿狅笍

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • Changelog is provided or not needed

@pr-triage pr-triage bot added the PR: draft label Jan 27, 2021
@codecov-io
Copy link

codecov-io commented Jan 27, 2021

Codecov Report

Merging #1054 (58daf90) into master (033bf9e) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1054      +/-   ##
=========================================
- Coverage    6.61%   6.61%   -0.01%     
=========================================
  Files         436     436              
  Lines       24137   24142       +5     
=========================================
  Hits         1596    1596              
- Misses      22541   22546       +5     
Impacted Files Coverage 螖
...nts/date-picker/internal/DatePickerDatePanel.razor 0.00% <0.00%> (酶)
...date-picker/internal/DatePickerDatetimePanel.razor 0.00% <0.00%> (酶)
components/date-picker/locale/DatePickerLocale.cs 0.00% <酶> (酶)

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 033bf9e...58daf90. Read the comment docs.

@ElderJames
Copy link
Member

Thank you, but I think it adds complexity. Can we try the implementation of the react version?

@anddrzejb
Copy link
Member Author

anddrzejb commented Jan 28, 2021

I was looking at react antdesign - and I cannot really make much sense out of it. It seem to me that DatePicker is getting its locale from rc-picker lib which in turn takes the short weekdays translation from days lib. The docs seems to say that the locale can be configured but do not say to what extent. I am not a react person, so that's that.
The solution I proposed seems to solve the mentioned problem + also solves the problem with choosing different first day of the week. Without this or other fix, the below configuration will not work:

    DatePickerLocale EN = new DatePickerLocale
    {
        FirstDayOfWeek = DayOfWeek.Wednesday,
        Lang = new DateLocale
        {
            YearFormat = "yyyy",
            MonthFormat = "MMM",
            DateSelect = "Select date",
            WeekSelect = "Select week",
            MonthSelect = "Select month",
            YearSelect = "Select year",
            QuarterSelect = "Select quarter",
            Today = "Today",
        }
    };

Event though FirstDayOfWeek is set to Wednesday, it will still start from Sunday (notice though that the dates are arranged correctly - if you just arrange the headings as requested, the dates are matching):
image

Moreover, if we change the locale settings to default for EN (FirstDayOfWeek = Sunday) and change the ShortWeekDays to start from Mo, this is happening
image

So I think there is a real problem here. As I said before, I am biased thus I will defend my solution, however adding a single field to manage this does not seem to me like a huge increase in complexity. And if this solution is going to be accepted, I will make appropriate changes to all locale json files, so we are going to have a lot of ground covered with default implementation.

@anddrzejb
Copy link
Member Author

@ElderJames so what are we going to do with this PR? Am I dropping all that was done here and only address the issue fix?

@ElderJames
Copy link
Member

The work is great, and can we bring the locale changes to an other PR and only fix #1045 here?
I want to merge the firstDayOfWeek and the MondayIndex.

@anddrzejb anddrzejb marked this pull request as ready for review February 4, 2021 18:16
@anddrzejb
Copy link
Member Author

@ElderJames Whenever you're ready 馃槃

@ElderJames ElderJames merged commit c0d4eba into ant-design-blazor:master Feb 5, 2021
@anddrzejb anddrzejb mentioned this pull request Apr 15, 2021
15 tasks
@anddrzejb anddrzejb deleted the datePickerDayOfWeek branch May 12, 2021 07:27
ElderJames pushed a commit that referenced this pull request Apr 23, 2022
* fix(module:datepicker): weekday header fix

* fix(module:rangepicker): reverse to only fix #1045
ElderJames pushed a commit that referenced this pull request Apr 30, 2022
* fix(module:datepicker): weekday header fix

* fix(module:rangepicker): reverse to only fix #1045
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DatePicker: When showTime=True, the dayofweek is wrong
3 participants