-
Notifications
You must be signed in to change notification settings - Fork 204
feat: implement style api for autosuggest component #4054
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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4054 +/- ##
=======================================
Coverage 96.99% 96.99%
=======================================
Files 862 862
Lines 25226 25226
Branches 9111 9111
=======================================
Hits 24469 24469
Misses 710 710
Partials 47 47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1fef73a to
e3f1bd9
Compare
| value: ['This is a test value'], | ||
| placeholder: [''], | ||
| disabled: [false, true], | ||
| readOnly: [false, true], |
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.
This creates more combinations than needed: 2x disabled, 2x readOnly, 2x invalid, and 2x style give 16 combinations, out of which 8 feature disabled=true and readOnly=true that are of little interest.
Besides, there are no permutations that show placeholder value or warning state.
I recommend declaring more atomic permutation groups, e.g.:
[
{ value: ['value'], disabled: [false, true], invalid: [false, true], warning: [false, true], style: [style] },
{ placeholder: ['Placeholder'], disabled: [false, true], invalid: [false, true], warning: [false, true], style: [style] },
{ value: ['value'], readOnly: [false, true], invalid: [false, true], warning: [false, true], style: [style] },
{ placeholder: ['Placeholder'], readOnly: [false, true], invalid: [false, true], warning: [false, true], style: [style] },
]
The same can be done twice if that is really necessary to cover multiple styles. Alternatively, we can be a little creative here and include a style selector component instead, also controlled with an url flag. Thus, opening a page with ?style=style1 will render one style, and using ?style=style2 will show an alternative style (provided there is actual value in having more than one).
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.
What do you think about this approach at /textarea/pseudo-selectors? That way we could interact with it in the screenshot tests (like hover, focus) more easily?
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.
I think the current approach with a single style is ok for now. The highly dynamic approach is good for manual testing or integration tests, but not so much for permutations. There, we can introduce maybe a single control to select out of a few available styles, while keeping the rest of the states predefined so to ensure good coverage with min configuration.
7798317 to
b46554e
Compare
Description
Extends the autosuggest component with a style API for customization, allowing developers to override default component styles.
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.