-
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
docs(contained-list): add usage examples #12621
Conversation
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Thanks Jan! This is looking great. ⭐️ Just a couple of comments:
- We are not going to support the Medium(40px) size, so we can take that one out.
- For the overflow menu hover states, can you make the hover state background go on top of the divider rules? This seems to be working right in the “With Actions” story https://deploy-preview-12621--carbon-components-react.netlify.app/?path=/story/components-containedlist--with-actions.
- For the usage example with the diagonal arrow/icon button in the header, instead could we just show a ghost icon button with an
add
icon? I know that this is a use case in the wild that people currently need so I would like to show that as another vetted example.
Medium 40px size:
Overflow menu button hover
Thank you! |
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 👍 ✅
@laurenmrice If it would be one end of the size spectrum I'd feel more comfortable dropping it but leaving out a size step would be a mistake imo. So yeah, I recommend offering the md/40px variant. |
Ok, let's add it! @janhassel |
Great! 👍 In this case I believe the PR is ready to be merged. |
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! Thanks Jan. ✅
Contributes to #12330
Adds a "Usage Examples" story showcasing the flexibility of the "Contained List" component based on what @laurenmrice shared in #12330.
Changelog
New