-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[Admin][UI] checkbox refactor #6275
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6275 +/- ##
==========================================
+ Coverage 88.93% 88.95% +0.01%
==========================================
Files 859 860 +1
Lines 18407 18423 +16
==========================================
+ Hits 16371 16388 +17
+ Misses 2036 2035 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
901894e to
12158f8
Compare
tvdeyen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice I have a simple wording nit.
| checked: @checked, | ||
| **@attributes | ||
| ) %> | ||
| <%= caption %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caption seems like a weirdly named attribute for the label. Can we name this duck as it quacks please? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah there was a reason why I chose to call it "caption" but I don't really remember now 😄 will change it to "label"
This is the first step to refactor checkbox component, so it can be used as a separate element or as a form control.
Motivation: We have a couple of places throughout the admin where checkboxes are used as part of a form, with all the same elements being reused to construct a checkbox field: - label with some caption; - hidden input field to support sending a "0" value in request; - optional toggletip hint; Few places however lacked the hidden input, therefore the form was not submitting the "0" value correctly. This updated component builds a reusable form checkbox, that encapsulates all the needed layout and logic for a form checkbox to be displayed and work properly, with an interface to customise it to each form needs.
"label" is more suitable here than "caption"
12158f8 to
c64ed78
Compare
tvdeyen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work
Note
Split from #6160
Summary
We have a couple of places throughout the admin where checkboxes are used as part of a form, with all the same elements being reused to construct a checkbox field:
Few places however lacked the hidden input, therefore the form was not submitting the "0" value correctly.
This updated component builds a reusable form checkbox, that encapsulates all the needed layout and logic for a form checkbox to be displayed and work properly, with an interface to customise it to each form needs.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs: