-
Notifications
You must be signed in to change notification settings - Fork 56
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
Star rating: Refactoring + rebranding. #2184
Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This comment was marked as outdated.
This comment was marked as outdated.
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.
If merged for v5.3.2, looks fine to me 🚀 Well done!
There's no visible focus when using the component with the keyboard in https://deploy-preview-2184--boosted.netlify.app/docs/5.3/forms/checks-radios/#star-rating for example |
Looks ok to me. |
This comment was marked as outdated.
This comment was marked as outdated.
In #1764, there was the following question:
Has it been tackled? What's the justification for not having a focused state? |
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.
LGTM
Before merging, please provide an answer to #2184 (comment). Our |
First thing to know is that checkboxes and radios don't have a That's why we had to mimic a Plain text is not focusable using One possible solution will be to add a Another solution will be to keep the same logic as Bootstrap (https://getbootstrap.com/docs/5.3/forms/checks-radios/#radios) and don't provide a Anything to add @julien-deramond and/or @Aniort? |
Sounds OK for me as-is. Preferred to ask because of things like https://getbootstrap.com/docs/5.3/forms/form-control/#readonly-plain-text where a readonly input can be focusable. |
Note: Please transform
- [ ]
into- (NA)
in the description when things are not applicableRelated issues
Closes #2005
Closes #1764
Closes #2183
Description
Use
mask-image
instead ofbackground-image
to make the dark mode easier.Might be merged before #2035.
Motivation & Context
The explanation in #2183. With all that changes, the bundlewatch for the star rating increase of 10kb (the size of the svg) and is up to
.4KB
.Types of change
Live previews
Checklist
Contribution
Accessibility
Design
Development
Documentation
Checklist (for Core Team only)
After the merge