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

Provide control over select props for each field type #49

Conversation

matt-koevort
Copy link
Contributor

🤔 This is a ...

  • New feature
  • Bug fix
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Refactoring
  • Code style optimization
  • Test Case
  • README update
  • Other (about what?)

🔗 Related issue link

💡 Background and solution

We have recently run into a problem where the onMouseDown event handler in antd/lib/select component is causing a modal (where the Cron component is rendered) to close. It has been found that this is due to the event bubbling. To solve this we can either:

  1. Explicitly set the onMouseDown to not propagate the event, or
  2. Provide more granular control over each select component that can be rendered by the Cron component

This PR addresses Option 2.

The Cron component can take a customSelectProps prop which is an object which has optional SelectProps definitions for each field type. These are then spread into each respective field component in Cron.tsx.

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Demo in storybook is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Tests are updated and passed without a decrease in coverage
    • NOTE this is dependant on this PR being merged to fix flaky tests
  • Format (lint & prettier) script passed
  • Build script is working
  • README API section is updated or not needed

We have recently run into a problem where the onMouseDown event handler in antd/lib/select component is causing a modal (where the Cron component is rendered) to close. It has been found that this is due to the event bubbling. To solve this we can either:
1. Explicitly set the onMouseDown to not propagate the event, or
2. Provide more granular control over each select component that can be rendered by the Cron component

This PR addresses Option 2.

The Cron component can take a `customSelectProps` prop which is an object which has optional `SelectProps` definitions for each field type. These are then spread into each respective field component in `Cron.tsx`.
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (5d8585e) 84.75% compared to head (f1f04a2) 84.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   84.75%   84.78%   +0.03%     
==========================================
  Files          12       12              
  Lines         492      493       +1     
  Branches      197      198       +1     
==========================================
+ Hits          417      418       +1     
  Misses         12       12              
  Partials       63       63              
Impacted Files Coverage Δ
src/fields/Hours.tsx 71.42% <ø> (ø)
src/fields/Minutes.tsx 64.28% <ø> (ø)
src/fields/MonthDays.tsx 61.53% <ø> (ø)
src/fields/Months.tsx 55.55% <ø> (ø)
src/fields/Period.tsx 56.75% <ø> (ø)
src/fields/WeekDays.tsx 50.00% <ø> (ø)
src/Cron.tsx 93.54% <100.00%> (+0.07%) ⬆️

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.

@xrutayisire
Copy link
Owner

Hello, thanks for the MR!
I let you handle prettier problems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants