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

feat(module: datepicker): add 12-hour time support #2501

Merged

Conversation

Alexbits
Copy link
Contributor

@Alexbits Alexbits commented Jun 3, 2022

Add 12-hour time support to the DatePicker/TimePicker/RangePicker

馃 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 3, 2022

@codecov
Copy link

codecov bot commented Jun 4, 2022

Codecov Report

Merging #2501 (ec041a1) into feature (1659aef) will increase coverage by 1.69%.
The diff coverage is 20.00%.

@@             Coverage Diff             @@
##           feature    #2501      +/-   ##
===========================================
+ Coverage    27.67%   29.37%   +1.69%     
===========================================
  Files          498      535      +37     
  Lines        33508    25851    -7657     
  Branches         0      258     +258     
===========================================
- Hits          9275     7595    -1680     
+ Misses       24233    18216    -6017     
- Partials         0       40      +40     
Impacted Files Coverage 螖
...date-picker/internal/DatePickerDatetimePanel.razor 0.00% <0.00%> (酶)
...e-picker/internal/DatePickerDatetimePanel.razor.cs 0.00% <0.00%> (酶)
...onents/date-picker/internal/DatePickerPanelBase.cs 48.57% <0.00%> (+6.71%) 猬嗭笍
.../date-picker/internal/DatePickerPanelChooser.razor 86.36% <酶> (+18.62%) 猬嗭笍
components/date-picker/internal/DatePickerBase.cs 47.36% <77.77%> (+2.14%) 猬嗭笍
components/date-picker/DatePicker.Razor.cs 68.75% <100.00%> (+0.82%) 猬嗭笍
components/date-picker/locale/DatePickerLocale.cs 100.00% <100.00%> (酶)
components/core/Helpers/MemberPath/PathNode.cs 46.15% <0.00%> (-8.85%) 猬囷笍
components/core/Reflection/TypeDefined.cs 77.77% <0.00%> (-7.94%) 猬囷笍
components/input/InputGroup.razor.cs 87.50% <0.00%> (-6.95%) 猬囷笍
... and 446 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 1659aef...ec041a1. Read the comment docs.

@ElderJames
Copy link
Member

ElderJames commented Jun 4, 2022

Thanks for contribute this @Alexbits , this make a great help!

In terms of globalization, I think AM and PM can use . NET built-in string for the current culture as the fallback.

  CultureInfo.CurrentCulture.DateTimeFormat.AMDesignator;
  CultureInfo.CurrentCulture.DateTimeFormat.PMDesignator

@Alexbits
Copy link
Contributor Author

Alexbits commented Jun 4, 2022

Thanks for contribute this @Alexbits , this make a great help!

In terms of globalization, I think AM and PM can use . NET built-in string for the current culture as the fallback.

  CultureInfo.CurrentCulture.DateTimeFormat.AMDesignator;
  CultureInfo.CurrentCulture.DateTimeFormat.PMDesignator

Thank you for your work @ElderJames ! I thought about this, but I was unsure if it was worth it since you provided the strings in the DateLocale class.

Where do you mean to implement this fallback?

@ElderJames
Copy link
Member

ElderJames commented Jun 5, 2022

Just the text in the input component, it can't be localized.

This is en-US:
image

This is zh-CN, you can switch the language on the right-top of the site.
image

The test code is:

<div>
  <TimePicker TValue="DateTime?" Use12Hours />

  AM: @System.Globalization.CultureInfo.CurrentCulture.DateTimeFormat.AMDesignator;
  PM: @System.Globalization.CultureInfo.CurrentCulture.DateTimeFormat.PMDesignator
</div>

Edit:

Ok, I found this to be due to formatting without setting current CultureInfo.

return value.ToString(format, CultureInfo.InvariantCulture);

We can change this line to below and it work.

value.ToString(format, CultureInfo)

And these two button we also need to be localize.

image

@Alexbits
Copy link
Contributor Author

Alexbits commented Jun 5, 2022

Hi @ElderJames, I checked the original ant.design, and AM/PM in the selection panel is not localized 馃

image

Also, in the ant-design-blazor the selection panel does not look right when AM/PM localized with the scrollbar visible:

image

@ElderJames
Copy link
Member

That's fine. We may need to modify the style to make the width adaptive.

@ElderJames
Copy link
Member

ElderJames commented Jun 7, 2022

There are an issue when I press the one of these buttons and they would switch by turns.
timepicker-12hours

@Alexbits
Copy link
Contributor Author

Alexbits commented Jun 8, 2022

There are an issue when I press the one of these buttons and they would switch by turns.

Fixed. Sorry! I've missed this somehow.

@Alexbits
Copy link
Contributor Author

Alexbits commented Jun 8, 2022

I'm considering implementing (separate PR) the time selection behavior as it is in the ant-design. When the OK button is present, the value is not applied until the OK button or Enter key is used to confirm it. Then it makes sense to have the OK button. What do you think about this @ElderJames?

@ElderJames
Copy link
Member

I'm considering implementing (separate PR) the time selection behavior as it is in the ant-design. When the OK button is present, the value is not applied until the OK button or Enter key is used to confirm it. Then it makes sense to have the OK button. What do you think about this @ElderJames?

Good idea. We just want to follow antd's behavior, and we also need to give the user the option to update the value when the panel is closed

ElderJames
ElderJames previously approved these changes Jun 8, 2022
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
Copy link
Member

For DatePicker, the display format in input has no AM/PM.

image

@Alexbits
Copy link
Contributor Author

Alexbits commented Jun 8, 2022

For DatePicker, the display format in input has no AM/PM.

Fixed

@Alexbits
Copy link
Contributor Author

Alexbits commented Jun 9, 2022

I also see another issue with the TimePicker. The selected time is often not visible. The TimePicker should auto-scroll itself when the selected value is not visible. 馃

@ElderJames
Copy link
Member

ElderJames commented Jun 10, 2022

I also see another issue with the TimePicker. The selected time is often not visible. The TimePicker should auto-scroll itself when the selected value is not visible. 馃

Yes, antd is designed to scroll to the top when time item clicked. We're not there yet. Do you have any ideas?

@Alexbits
Copy link
Contributor Author

I also see another issue with the TimePicker. The selected time is often not visible. The TimePicker should auto-scroll itself when the selected value is not visible. 馃

Yes, antd is designed to scroll to the top when time item clicked. We're not there yet. Do you have any ideas?

I'm looking into it.

@ElderJames ElderJames merged commit acb6c89 into ant-design-blazor:feature Jun 10, 2022
@Alexbits Alexbits deleted the feat/time-picker-12-hour branch June 10, 2022 12:37
ElderJames pushed a commit that referenced this pull request Sep 6, 2022
* feat(module: datepicker): add 12-hour time support

* fix(module: datepicker): revert 24-hour time format

* fix(module: datepicker): AM/PM not localized in the date input component

* fix(module: datepicker): AM/PM in the selection panel are not localized

* Update components/date-picker/internal/DatePickerDatetimePanel.razor

Co-authored-by: James Yeung <shunjiey@hotmail.com>

* fix(module: datepicker). AM/PM switches toggling incorrectly

* fix(module: datepicker): 24-hour format in DatePicker when Use12Hours

* fix(module: datepicker): time format in docs

* refactor(module: datepicker): code cleanup

* feat(module: datepicker): add 12-hour format to en-US locale
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