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.

Sign up for GitHub

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

Jump to bottom

feat(module: datepicker): add mask to DatePickerBase for input value constraint #3120

Merged
merged 23 commits into from
Oct 5, 2023
Merged

feat(module: datepicker): add mask to DatePickerBase for input value constraint #3120

merged 23 commits into from
Oct 5, 2023

Conversation

agolub-s
Copy link
Contributor

@agolub-s agolub-s commented Feb 20, 2023 •
edited

🤔 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

You have documentation for multiple input formats, but it is not working.
Solution: Added support multiple formats for input in DatePciker.

📝 Changelog

Language Changelog
🇺🇸 English Add support multiple formats for input in DatePciker
🇨🇳 Chinese 为Datepicker添加支持多种格式

☑️ 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 Feb 20, 2023 •
edited

@agolub-s agolub-s mentioned this pull request Feb 20, 2023
Merged
15 tasks
@codecov
Copy link

codecov bot commented Feb 20, 2023 •
edited

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d924e1c) 46.64% compared to head (c9ffbfd) 46.78%.
Report is 1 commits behind head on feature.

Additional details and impacted files 
@@             Coverage Diff             @@
##           feature    #3120      +/-   ##
===========================================
+ Coverage    46.64%   46.78%   +0.14%     
===========================================
  Files          558      559       +1     
  Lines        27068    27112      +44     
  Branches       276      276              
===========================================
+ Hits         12626    12685      +59     
+ Misses       14402    14387      -15     
  Partials        40       40
Files Coverage Δ
components/core/Helpers/Formatter.cs 75.00% <100.00%> (-2.78%) ⬇️
components/date-picker/DatePicker.razor 77.77% <ø> (ø)
components/date-picker/RangePicker.razor 54.54% <ø> (ø)
components/date-picker/RangePicker.razor.cs 70.03% <ø> (-0.95%) ⬇️
...ponents/date-picker/internal/DatePickerInput.razor 91.89% <100.00%> (+0.22%) ⬆️
components/locale-provider/LocaleProvider.cs 73.75% <100.00%> (+13.49%) ⬆️
components/slider/Slider.razor 100.00% <100.00%> (ø)
components/date-picker/DatePicker.Razor.cs 79.41% <75.00%> (-2.12%) ⬇️
components/date-picker/internal/DatePickerBase.cs 82.37% <96.96%> (+1.45%) ⬆️
components/date-picker/locale/FormatAnalyzer.cs 90.29% <80.00%> (-0.31%) ⬇️
... and 1 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agolub-s
Copy link
Contributor Author

Hello, is anyone here?

@ElderJames ElderJames mentioned this pull request Jul 31, 2023
Closed
@agolub-s agolub-s force-pushed the feature/datepicker_multiple_formats branch from cb4d425 to 363781e Compare August 15, 2023 10:05
ElderJames
ElderJames reviewed Aug 15, 2023
View reviewed changes
components/date-picker/RangePicker.razor.cs Outdated Show resolved Hide resolved
components/date-picker/RangePicker.razor.cs Outdated Show resolved Hide resolved
components/date-picker/internal/DatePickerBase.cs Outdated Show resolved Hide resolved
@agolub-s agolub-s changed the title DatePicker multiple formats feat(module: datepicker): add mask to DatePickerBase for input value constraint Aug 17, 2023
@ElderJames ElderJames mentioned this pull request Aug 22, 2023
Open
ElderJames
ElderJames previously approved these changes Sep 21, 2023
View reviewed changes
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.

Thank you @agolub-s !

@ElderJames ElderJames changed the base branch from master to feature September 21, 2023 16:23
ElderJames
ElderJames reviewed Sep 21, 2023
View reviewed changes
components/date-picker/RangePicker.razor.cs
Comment on lines -237 to -240
else
{
_pickerStatus[index].SelectedValue = null;
}
Copy link
Member

Choose a reason for hiding this comment

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

I noticed this. Did we miss it?

Copy link
Member

Choose a reason for hiding this comment

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

This is add from your previous PR https://github.com/ant-design-blazor/ant-design-blazor/pull/3087/files#diff-9cc53059c159df4e894628ddb114efb40c2bfec6a051992a57a8a31b16215dd1R239

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks, I'm trying to fix it. The tests are not repeatable. I'll stabilize it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it, but with some other changes. At now start tests multiple times with multithreading does not fail on my machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ElderJames test coverage has dropped, what do you think about async methods in DatePicker, I have to revert this or use async overloads in tests?

Copy link
Member

Choose a reason for hiding this comment

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

I would like not changing to asynchronous just yet, because the changes are already substantial. We can perform that in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted changes with async

Copy link
Member

Choose a reason for hiding this comment

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

Thank you

@ElderJames ElderJames dismissed their stale review September 21, 2023 16:43

Still a little confused.

@agolub-s agolub-s force-pushed the feature/datepicker_multiple_formats branch from 8dbd44f to c9ffbfd Compare September 27, 2023 09:52
@ElderJames
Copy link
Member

Hello @Alexbits , could you please help me to review this PR? This would be cuase conflicts with your latest PR.

@Alexbits
Copy link
Contributor

Hello @Alexbits , could you please help me to review this PR? This would be cuase conflicts with your latest PR.

Hey @ElderJames This PR is confusing. I cannot help this further. I do not work with russians.

@ElderJames ElderJames merged commit 8e60219 into ant-design-blazor:feature Oct 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElderJames ElderJames ElderJames approved these changes

Assignees
No one assigned
Labels
PR: unreviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@agolub-s @ElderJames @Alexbits