-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(structuredList): update to use radio button icon, radios are left aligned under featureflag #15910
feat(structuredList): update to use radio button icon, radios are left aligned under featureflag #15910
Conversation
✅ 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. |
✅ 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. |
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @Kritvi-bhatia17 : Can I get your design review on this . |
Yes! sure @2nikhiltom will do that |
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.
Feature flags should be used to modify component source code. The feature flag stories for that component would then show the result of the flag being turned on and the modified behavior of the component source code.
Is the current checkmark functionality built into any of the components' source, or does it require the consumer to compose the checkmark into the proper location within StructredListCell?
If it's already on the consumer to compose this properly, we wouldn't need a feature flag. We could just update the existing stories to show the new icon being used on the right.
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.
Looks good to me from design perspective!!
Nice work @2nikhiltom 🔥
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.
Hii @2nikhiltom! after speaking with @laurenmrice , I realized that we need to add a prop for both with and without background options under the 'Structured Lists' section for both 'Default' and 'Selection'.
I just pushed an update here that adds the feature flag to modify the styling, and also added a new One piece remaining is to ensure the blue focus ring is applied to the row on click in addition to on keyboard navigation. Keyboard nav already gets the focus ring, so I think it might be just a change in how we apply focus styling |
@2nikhiltom I think I got the focus ring working actually, take a look and let me know what you think |
The PublicAPI snapshot is failing, just needs a |
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 is looking great, just a couple things to change and I think this will be good to go.
packages/react/src/components/StructuredList/StructuredList.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/components/StructuredList/StructuredList.tsx
Outdated
Show resolved
Hide resolved
packages/styles/scss/components/structured-list/_structured-list.scss
Outdated
Show resolved
Hide resolved
packages/styles/scss/components/structured-list/_structured-list.scss
Outdated
Show resolved
Hide resolved
Hey @tay1orjones , thanks for the review. |
Hey @tay1orjones , @andreancardona : it seems like the problem is still there even after running "yarn test -u" and pushing changes. Any ideas on what might be missing? |
@2nikhiltom based on the logs, it was failing on a snapshot change unrelated to the changes you made here. Generally that signals that the PR needs to be updated with the latest from I pressed the "update with latest" button on the PR interface here, pulled down the PR locally, ran |
@Kritvi-bhatia17 @andreancardona this is ready for your review |
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.
looks good to me - nice job @2nikhiltom !
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.
Great work Nikhil!! 🔥
b9abee7
Hey there! v11.54.0 was just released that references this issue/PR. |
…t aligned under featureflag (carbon-design-system#15910) * feat(structuredList): use of radio button icon,radios are left aligned * chore: revert few typos * feat(structuredList): adds feature flag story for structuredList * feat(structuredlist): add radio button selection, styling behind a flag * fix(structuredlist): add focus ring on click * chore(StructuredList): remote JS feature flag, delete unnecessary styles * chore(StructuredList): revert story to use checkmark icons * fix(StructuredList): adds onclick focus ring with selection prop * fix(StructuredList): adds 'selection' props * fix(structuredlist): focus on click & Tab,handles clickaway events * chore(StructuredList): add typescript types on refs * chore(StructuredList): updates PublicAPI snapshot * chore: update snapshots * fix(StructuredList): focus rings only on selection prop * chore: update snaps --------- Co-authored-by: Taylor Jones <[email protected]> Co-authored-by: Andrea N. Cardona <[email protected]> Co-authored-by: Taylor Jones <[email protected]>
Closes #15479 & #15931
New
Changed
Testing / Reviewing