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: RangePicker): Always run default disabled logic, even with custom function provided #2947

Conversation

wss-kroche
Copy link
Contributor

@wss-kroche wss-kroche commented Dec 14, 2022

We ran into the need to use custom disabled logic, but also wanted to keep the default logic that kept the start/end in the proper order.

This PR adds a property to allow that setting which "mode" you want to use and then wraps the DisabledDate function provided in a method to check the type of check to run. It then either runs one or both.

Through discussion below, this PR updates the disabled logic to always apply the default logic even when custom is provided.

🤔 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 Update RangePicker disabled date logic to always apply default logic even when custom is provided. This keeps ranges in the proper order even with custom disabled logic.
🇨🇳 Chinese 更新 RangePicker 禁用日期逻辑以始终应用默认逻辑,即使在提供自定义时也是如此。即使使用自定义禁用逻辑,这也会使范围保持正确的顺序。

☑️ 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 Dec 14, 2022

@wss-kroche wss-kroche force-pushed the feature/736345-range-picker-default-and-custom-disabled branch from 314d426 to 0bded29 Compare December 14, 2022 21:45
@ElderJames
Copy link
Member

Thanks for contribution @wss-kroche , I think we can implement 'Disabled Date & Time' as antd directly.

@wss-kroche
Copy link
Contributor Author

Thanks for contribution @wss-kroche , I think we can implement 'Disabled Date & Time' as antd directly.

I'm not sure I understand what you mean. Disabled date/time is implemented in ant and in ant blazor, but there is default logic for that disabled date for range picker that keeps the dates in the correct order. We need custom logic for disabled date, but we need the default as well because if you pick the dates in the wrong order the component completely breaks too. This is adding the option to override the default logic or use both.

@ElderJames
Copy link
Member

Thanks for contribution @wss-kroche , I think we can implement 'Disabled Date & Time' as antd directly.

I'm not sure I understand what you mean. Disabled date/time is implemented in ant and in ant blazor, but there is default logic for that disabled date for range picker that keeps the dates in the correct order. We need custom logic for disabled date, but we need the default as well because if you pick the dates in the wrong order the component completely breaks too. This is adding the option to override the default logic or use both.

Ok, I see it. But I don't get the idea why do you need "Custom Disabled Only".

@wss-kroche
Copy link
Contributor Author

Thanks for contribution @wss-kroche , I think we can implement 'Disabled Date & Time' as antd directly.

I'm not sure I understand what you mean. Disabled date/time is implemented in ant and in ant blazor, but there is default logic for that disabled date for range picker that keeps the dates in the correct order. We need custom logic for disabled date, but we need the default as well because if you pick the dates in the wrong order the component completely breaks too. This is adding the option to override the default logic or use both.

Ok, I see it. But I don't get the idea why do you need "Custom Disabled Only".

Only to give flexibility to allow running custom, default or both. Do you think the default should always run? I wasn't sure if it should always run or not but if it should we can just make it always run and use the DisabledDate as an additional one to run. That would be a breaking change though and need to go to feature as well I think.

@ElderJames
Copy link
Member

ElderJames commented Dec 16, 2022

Do you think the default should always run?

Yeah, I think that's more close to antd's behavior.

That would be a breaking change though and need to go to feature as well I think.

This can be seen as a bug, so we can keep it all at master.

@wss-kroche wss-kroche force-pushed the feature/736345-range-picker-default-and-custom-disabled branch 2 times, most recently from f6b6af7 to a838af9 Compare January 3, 2023 15:05
@wss-kroche wss-kroche changed the title feat(module: RangePicker): Add ability to use both default and custom… feat(module: RangePicker): Always run default disabled logic, even with custom function provided Jan 3, 2023
@wss-kroche wss-kroche changed the base branch from master to feature January 3, 2023 15:07
@wss-kroche wss-kroche changed the base branch from feature to master January 3, 2023 15:09
@wss-kroche
Copy link
Contributor Author

Do you think the default should always run?

Yeah, I think that's more close to antd's behavior.

That would be a breaking change though and need to go to feature as well I think.

This can be seen as a bug, so we can keep it all at master.

I've updated the logic to always apply default and updated the change comment

… default logic even when custom is provided. This keeps ranges in the proper order even with custom disabled logic.
@wss-kroche wss-kroche force-pushed the feature/736345-range-picker-default-and-custom-disabled branch from a838af9 to ae67f45 Compare January 3, 2023 15:13
@wss-kroche wss-kroche changed the title feat(module: RangePicker): Always run default disabled logic, even with custom function provided fix(module: RangePicker): Always run default disabled logic, even with custom function provided Jan 3, 2023
@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Base: 43.61% // Head: 45.01% // Increases project coverage by +1.39% 🎉

Coverage data is based on head (ae67f45) compared to base (c2b0fd4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2947      +/-   ##
==========================================
+ Coverage   43.61%   45.01%   +1.39%     
==========================================
  Files         551      551              
  Lines       25918    25915       -3     
  Branches      263      263              
==========================================
+ Hits        11305    11666     +361     
+ Misses      14573    14209     -364     
  Partials       40       40              
Impacted Files Coverage Δ
components/input/Input.cs 68.81% <ø> (+0.02%) ⬆️
components/date-picker/RangePicker.razor.cs 65.67% <100.00%> (+0.45%) ⬆️
components/date-picker/internal/DatePickerBase.cs 79.01% <100.00%> (+0.66%) ⬆️
components/drawer/Drawer.razor.cs 90.25% <0.00%> (-3.25%) ⬇️
components/core/Component/Overlay/Overlay.razor.cs 73.83% <0.00%> (-2.91%) ⬇️
components/select/Select.razor.cs 53.24% <0.00%> (+0.26%) ⬆️
components/select/SelectBase.cs 71.19% <0.00%> (+0.27%) ⬆️
components/core/CssSizeLength.cs 86.07% <0.00%> (+1.07%) ⬆️
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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 a2fbb93 into ant-design-blazor:master Jan 4, 2023
kooliokey pushed a commit to kooliokey/ant-design-blazor that referenced this pull request Jan 15, 2023
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