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

feat(module:datepicker): Scroll to selected time in DatePicker/TimePicker #2512

Merged

Conversation

Alexbits
Copy link
Contributor

@Alexbits Alexbits commented Jun 10, 2022

🤔 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

💡 Background and solution

📝 Changelog

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

@github-actions
Copy link

github-actions bot commented Jun 10, 2022

@Alexbits
Copy link
Contributor Author

Hey @ElderJames. I've implemented auto-scroll behavior similar to the one in ant.design. It may need a smoother animation when the selection panel (overlay) is open, but it generally works okay. Please check and let me know what you think.

@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #2512 (dcff8bb) into feature (acb6c89) will increase coverage by 1.68%.
The diff coverage is 19.13%.

@@             Coverage Diff             @@
##           feature    #2512      +/-   ##
===========================================
+ Coverage    27.68%   29.37%   +1.68%     
===========================================
  Files          498      535      +37     
  Lines        33526    25948    -7578     
  Branches         0      260     +260     
===========================================
- Hits          9283     7621    -1662     
+ Misses       24243    18287    -5956     
- Partials         0       40      +40     
Impacted Files Coverage Δ
components/calendar/Calendar.razor.cs 0.00% <0.00%> (ø)
...s/core/JsInterop/modules/dom/manipulationHelper.ts 13.69% <0.00%> (ø)
components/date-picker/RangePicker.razor.cs 0.00% <0.00%> (ø)
...date-picker/internal/DatePickerDatetimePanel.razor 0.00% <0.00%> (ø)
...e-picker/internal/DatePickerDatetimePanel.razor.cs 0.00% <0.00%> (ø)
...s/date-picker/internal/DatePickerTemplate.razor.cs 31.81% <0.00%> (+2.47%) ⬆️
components/date-picker/internal/DatePickerBase.cs 46.88% <20.00%> (+1.21%) ⬆️
components/core/JsInterop/JSInteropConstants.cs 28.33% <33.33%> (-0.49%) ⬇️
components/date-picker/locale/FormatAnalyzer.cs 90.59% <53.57%> (-7.33%) ⬇️
components/date-picker/DatePicker.Razor.cs 68.96% <100.00%> (+0.89%) ⬆️
... and 450 more

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 acb6c89...dcff8bb. Read the comment docs.

@ElderJames
Copy link
Member

Well done! You can refer to BackTop component that it scroll more smoothly.

And I find that it wouldn't scroll when I modify the time value directly:

date-picker-time-scroll-top

@Alexbits
Copy link
Contributor Author

Well done! You can refer to BackTop component that it scroll more smoothly.

And I find that it wouldn't scroll when I modify the time value directly:

There are other issues with manual input.

  • DatePicker.Razor.cs.OnBlur resets the value to the cached if Enter key is not pressed, the Ok button does nothing.
  • The time string with AM/PM is not parsed by FormatAnalyzer.TryPickerStringConvert and the value is not changed as result.

@Alexbits
Copy link
Contributor Author

Alexbits commented Jun 11, 2022

  • DatePicker.Razor.cs.OnBlur resets the value to the cached if Enter key is not pressed, the Ok button does nothing./

I've tried changing it to accept parsed value immediately 9211cf8, but in this case, a unit test fails. So what behavior do we want here? Should it apply the value of manual input with Enter key and Ok button only? But in this case, why is the input from the panel confirmed immediately? 🤔 In the ant.design the input is applied only after Enter key or Ok button.

@ElderJames
Copy link
Member

I've tried changing it to accept parsed value immediately 9211cf8, but in this case, a unit test fails. So what behavior do we want here? Should it apply the value of manual input with Enter key and Ok button only? But in this case, why is the input from the panel confirmed immediately? 🤔 In the ant.design the input is applied only after Enter key or Ok button.

Maybe we can follow the behavior of ant.design

Copy link
Member

@ElderJames ElderJames left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@ElderJames ElderJames merged commit 8585bba into ant-design-blazor:feature Jun 15, 2022
@Alexbits Alexbits deleted the feat/datepicker-selected-time branch June 15, 2022 15:53
ElderJames pushed a commit that referenced this pull request Sep 6, 2022
…icker (#2512)

* feat(module: datetimepicker): scroll to selected time

* refactor(module: datepicker): move scroll to code behind

* fix(module: datetimepicker): no scroll to selected time on manual input

* fix(module: datetimepicker): 12-hour time is not parsed on manual input

* fix(module: datetimepicker): entered time ignored on manual input

* feat(module: datepicker): add animation to time picker

* Revert "fix(module: datetimepicker): entered time ignored on manual input"

This reverts commit 9211cf8.

* refactor(module: datepicker): cleanup redundant code

* refactor(module: datepicker): reduce size of animation function

* fix(module: datepicker):duplicate calls to StateHasChanged on date hove
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.

None yet

2 participants