-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(select): resolve parity issues #18367
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18367 +/- ##
=======================================
Coverage 84.17% 84.17%
=======================================
Files 408 408
Lines 14435 14435
Branches 4690 4694 +4
=======================================
Hits 12150 12150
Misses 2120 2120
Partials 165 165 ☔ View full report in Codecov by Sentry. |
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.
Hey @makafsal , this is looking good! Just had a couple of comments:
Default story
- There should be no helper text shown in the field in WC to match React.
- When the select menu is open, can we match the menu options to be exactly the same as the React one? It would be good to try to keep them the same.
- I do not see the
decorator
,noLabel
, orslug
props in WC but they are present in React. Just clarifying if that is intentional? - I see
onInput
,placeholder
, andvalue
props in Web components but not in React. Also clarifying if that is intentional that they do not match between the two? - When the prop
inline
is set to true and the propdisabled
is set to true, there should be no background on the field, it should be transparent. - There is a bug when the prop
inline
is set to true and the propwarn
is set to true. The warning icon should be 8px to the left of the chevron icon. I do want to note that this scenario in React is currently really broken, which I am making a separate issue for. But I feel that we can address the WC spacing problem here in this PR.
Inline story
- There should be no helper text shown in the field in WC to match React.
- When the select menu is open, can we match the menu options to be exactly the same as the React one? It would be good to try to keep them the same.
- When the prop
disabled
is set to true, there should be no background on the field, it should be transparent. - There is a slight bug when the prop
inline
is set to true and the propwarn
is set to true. The warning icon should be 8px to the left of the chevron icon.
With AI Label
- There should be no helper text shown in the field in WC to match React.
- The input field is missing the vertical divider between the AI slug and the chevron icon. You can view the design specs here on our website.
- I do not see the
decorator
,noLabel
, orslug
props in WC but they are present in React. Just clarifying if that is intentional? - I do see the
onInput
,placeholder
, andvalue
props in WC but not React. Also is this intentional? - The WC AI explainability popover design isn’t matching the React one. I have documented the design specs in this image below.
*Note: I will be making a separate bug issue to tackle some unrelated visual bugs that were already in React.
Closes #17931 #18164
Resolved a few parity issues
Changelog
Changed
Testing / Reviewing
select
playground storybookselect
element using the browser dev tools and select anyoption
by putting theselected
attributevalue
prop from controls to give thevalue
prop to the componentThis way we can verify all cases mentioned in the table.
with AI label
sub-story from theselect
storybookwith AI label
sub-story from theselect
storybooktab
button and compare the focus border thickness with the corresponding React storybookNote Also verify this style from the
AI Label
component storybook