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

fix(module:DatePicker&RangePicker): OneOf to TValue, default value for picker, optimizations #933

Merged
merged 18 commits into from
Jan 10, 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

  1. Fixes DatePicker does not focus on either DefaultValue or Value dates after initialization. #906
  2. Inconsistent data types: Value was generic but DefaultValue and DefaultPickerValue were OneOf<DateTime, DateTime[]>. That approach was allowing calls like this:
<RangePicker TValue="DateTime" Format="yyyy/MM/dd"  DefaultPickerValue="new DateTime[] { new DateTime(2020, 10, 10), new DateTime(2021, 12, 10) }"/>
<DatePicker TValue="DateTime" Format="yyyy/MM/dd"  DefaultPickerValue="new DateTime[] { new DateTime(2020, 10, 10), new DateTime(2021, 12, 10) }"/>

and there was no warning from the compiler. However during runtime NullReferenceException was being thrown due to inconsistent data types.
3. DatePicker and RangePicker were handling the same type of events in a different way: in RangePicker there is a separate OnInputClick method to handle OnClick event. In DatePicker there is no such method, instead this code await _dropDown.Show(); ChangeFocusTarget(true, false); is executed.
4. (Appeared after fixing #906) For RangePicker a 2-panel picker is shown for each input. Every 2-panel picker should set its 1st panel to a default value and 2nd panel should be pointing to next period (for month view - next month, for year view - next decade, etc). The method to pick up values for panels were taking both values for both panels. So if Value = { "2020-10-10", "2021-10-10" }, then for:

  • (inputStart) 1st panel was showing "2020-10-10",
  • (inputStart) 2nd was showing "2021-10-10" (instead of 1st panel + 1 month)
  • (inputEnd) 1st panel was showing "2020-10-10" (instead of "2021-10-10"),
  • (inputEnd) 2nd was showing "2021-10-10" (instead of 1st panel + 1 month)
    The analysis was made by comparing how ant-design react is behaving.
  1. Different combinations of Value, DefaultValue, DefaultPickerValue were not returning expected results (in comparison to what ant-design react is returning).
  2. In case of RangePicker Values are not sorted, so a picker could be shown with start set after the end. The values are sorted by ant-design react.

💡 Background and solution

When picker is shown, data is fetched from PickerValue. That value was not properly initiated. That value should be initiated during initialization of the the component, and then changed whenever a new value is selected. However the value(s) in PickerValue can be different depending on combination of Value, DefaultValue, DefaultPickerValue. A new static class RangePickerDefaults has been introduced to handle RangePicker.PickerValue scenarios (includes also tests covering most of the combinations). For DatePicker evaluation of the PickerValue is simple and was included in the ProcessDefaults() method that is called in the OnInitialized() method.
The default evaluation follows the table (based on analysis of the ant-design react)(forgive my excel skills...):
datePickerChart
Ad. 2. OneOf<DateTime, DateTime[]> was replaced with generic TValue.
### This is a breaking change
For example, due to the change to TValue, the docs files Disabled.razor and Format.razor (check commit 3c2024c) had to be altered because the compiler was raising an error. I do think this is desirable, but if this is going to be accepted, should probably be noted in the changelog.
Ad. 3. Handling of the events had been unified (commit d03518e)
Ad. 4. This change required fixes in multiple files. A change was required to actually rely on current active input index instead of on passed index value. It is still allowed to pass the index value (and is useful in certain scenarios) but most scenarios actually no longer send this value.
As a side note: commit 076c77f changed index value for picker to hardcoded value 0 - this is for DatePicker where only index==0 is used, so to avoid any mistake I made it clear this refers only to a single (first) value.
Ad. 6. SortValue method was added to DatePickerBase class. It is called whenever Value, DefaultValue or DefaultPickerValue is set/changed. It also required to alter IsNullable evaluation.

Commit 72b3067 addresses certain optimization issue I discovered: pickers seems to be initialized multiple times. I could not track all the reasons of multiple initializations, but I found that not changing MaxCol, MaxRow and ViewStartDate limitted a bit that number. Still, there is still a lot of things happening, even when picker is hidden (after first time it is shown - the dates for pickers seem to be constantly recalculated on each mouse move even if picker is not visible - this does require more research).

In this PR I multiple times referred to original ant-design for react as my reference whenever I was not sure how should the picker in question behave. Most of the times, I was trying to follow the original. That is why I created the chart. The choices made by the original ant-design for the behaviours are justifiable - even if someone argues that a certain result is not what he/she expected, it is still logically explainable. However certain values are not identical - for example the minimum value. This is because Javascript is a weakly typed language, so there is no differentiation between nullable and non-nullable (at least not to my knowledge). So I was trying to follow the logic that if it is not nullable, then it is minimum. Therefore the presence of minimum that does not really exists in ant-design react (unless manually set as a value).
Second issue I had with original ant-design react - range picker does not follow the logic of date picker. To be honest, I do not know what logic it follows... I couldn't figure it out (granted, I did not spent a long time on it). So I decided to actually follow the same logic as for DatePicker (I used the chart) - this seemed most intuitive. I do not think using a different logic for DatePicker and RangePicker would add to user experience...

📝 Changelog

DefaultValue and DefaultPickerValue have to be of the same type as Value, but still can be null or can be omitted.

Language Changelog
🇺🇸 English x
🇨🇳 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

@ElderJames
Copy link
Member

ElderJames commented Dec 29, 2020

Thanks for contribution @anddrzejb . We may need some time to review it.

@anddrzejb
Copy link
Member Author

I would be surprised if you just accepted it 😄. I'll be waiting for comments/input/changes/etc.

@mutouzdl
Copy link
Member

/preview

@github-actions
Copy link

github-actions bot commented Dec 30, 2020

@mutouzdl
Copy link
Member

mutouzdl commented Jan 4, 2021

@anddrzejb Thanks for your pr!

It's great, but i found an issue here.
I selected the start date(2020.07.09), and than it focus on the end date input(It's correct). But the picker panel's date was wrong:
image

It should be '2020/08', like this:
image

@anddrzejb
Copy link
Member Author

@mutouzdl thanks for pointing that out. I fixed it. When checking I also noticed some other issues. My second commit addresses the scenario when Value is null and DefaultValue is set. In that scenario, when any date is selected, Value get initialized to empty array of dates with single index set to newly selected value. The other index is null/min. The fix is setting the other index to whatever is in DefaultValue.

It seems the check has failed in the areas that I have not touched. The app is building and running successfully on my machine. I run all the tests present in the app and they were successful.

@mutouzdl
Copy link
Member

mutouzdl commented Jan 6, 2021

@anddrzejb Hi, here has 1 failing check
image

@anddrzejb
Copy link
Member Author

anddrzejb commented Jan 6, 2021

@mutouzdl yeah, I mentioned that. Last 4 PR have exactly the same check fail - there seems to be a problem with IsExternalInit (whatever that is). My local dev stopped compiling on that once I pulled changes from master. I know this is not on me as I have not change anything in there...

@ElderJames
Copy link
Member

I will fix it later today.

@codecov-io
Copy link

Codecov Report

Merging #933 (0e769b0) into master (55cfc77) will increase coverage by 0.26%.
The diff coverage is 23.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #933      +/-   ##
=========================================
+ Coverage    5.49%   5.75%   +0.26%     
=========================================
  Files         405     406       +1     
  Lines       21532   21702     +170     
=========================================
+ Hits         1184    1250      +66     
- Misses      20348   20452     +104     
Impacted Files Coverage Δ
components/date-picker/DatePicker.Razor.cs 0.00% <0.00%> (ø)
components/date-picker/DatePicker.razor 0.00% <0.00%> (ø)
components/date-picker/RangePicker.razor.cs 0.00% <0.00%> (ø)
components/date-picker/internal/DatePickerBase.cs 0.00% <0.00%> (ø)
...onents/date-picker/internal/DatePickerPanelBase.cs 0.00% <0.00%> (ø)
.../date-picker/internal/DatePickerPanelChooser.razor 0.00% <0.00%> (ø)
...s/date-picker/internal/DatePickerTemplate.razor.cs 0.00% <0.00%> (ø)
...nts/date-picker/internal/DatePickerYearPanel.razor 0.00% <0.00%> (ø)
components/date-picker/RangePickerDefaults.cs 100.00% <100.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 55cfc77...0e769b0. Read the comment docs.

@ElderJames ElderJames merged commit 94468e2 into ant-design-blazor:master Jan 10, 2021
@anddrzejb anddrzejb deleted the datePickerInitialValue branch May 12, 2021 07:27
ElderJames added a commit that referenced this pull request Apr 23, 2022
…ptimizations (#933)

* fix(module:DatePicker): input OnClick has new event handler

* fix(module:DateTime): remove misleading reliance on picker index

* fix(module: DatePicker & RangePicker): DefaultValue type change

DefaultValue type change to align with Value type

* fix(module:rangepicker): add default values helper with tests

* fix(module:datepicker): ChangePickerValue action declaration fix

Picker value is served based on input index (start/end)

* fix(module:DatePicker): optimization

ViewStartDate, MaxRow & MaxCol do not cause refresh if not changed

* fix(module:DatePicker): min date fix

ArgumentOutOfRangeException fix for dates before DateTime.MinValue

* fix(module:RangePicker): sorted values

Values get ordered on set

* fix(module:DatePicker): OneOf switch to TValue

* fix(module:DatePicker): code optimization and PickerValue fix

PickerValue fix gets first panel value and evaluates second panel value

* fix(module:DatePicker): default values evaluation

* fix(module:RangePicker): default picker value fix

* docs(module:DatePicker): switch to TValue for DefaultValue

* fix(module:RangePicker): other value picker fix

* fix(module:RangePicker): on value init include DefaultValue

Co-authored-by: James Yeung <shunjiey@hotmail.com>
ElderJames added a commit that referenced this pull request Apr 30, 2022
…ptimizations (#933)

* fix(module:DatePicker): input OnClick has new event handler

* fix(module:DateTime): remove misleading reliance on picker index

* fix(module: DatePicker & RangePicker): DefaultValue type change

DefaultValue type change to align with Value type

* fix(module:rangepicker): add default values helper with tests

* fix(module:datepicker): ChangePickerValue action declaration fix

Picker value is served based on input index (start/end)

* fix(module:DatePicker): optimization

ViewStartDate, MaxRow & MaxCol do not cause refresh if not changed

* fix(module:DatePicker): min date fix

ArgumentOutOfRangeException fix for dates before DateTime.MinValue

* fix(module:RangePicker): sorted values

Values get ordered on set

* fix(module:DatePicker): OneOf switch to TValue

* fix(module:DatePicker): code optimization and PickerValue fix

PickerValue fix gets first panel value and evaluates second panel value

* fix(module:DatePicker): default values evaluation

* fix(module:RangePicker): default picker value fix

* docs(module:DatePicker): switch to TValue for DefaultValue

* fix(module:RangePicker): other value picker fix

* fix(module:RangePicker): on value init include DefaultValue

Co-authored-by: James Yeung <shunjiey@hotmail.com>
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 does not focus on either DefaultValue or Value dates after initialization.
4 participants