-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: added tags with overflow handler #19427
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
base: main
Are you sure you want to change the base?
feat: added tags with overflow handler #19427
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
dfb29de
to
29ed8ca
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19427 +/- ##
==========================================
+ Coverage 84.48% 84.51% +0.02%
==========================================
Files 373 373
Lines 14645 14708 +63
Branches 4790 4836 +46
==========================================
+ Hits 12373 12430 +57
- Misses 2125 2130 +5
- Partials 147 148 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
looking good! just a few changes.
1. Update left padding for tags
To maintain type alignment tabs need to hang a bit to the left of the grid $spacing-05
2. Focus stuck
When using the keyboard, the ficus gets stuck on monitoring. Could just be me but wanted to double check!
3. Update tag size for storybook
Not a bug, but we would like people to use medium tag size, so could the default size be medium in storybook?
Question:
Just wanted to double check on collapse button. we decided to keep an open slot for teams to insert a button if needed and just wanted to make sure that was there. From Anna:
For the page actions collapsible behavior, I'm thinking we currently support just buttons for now as it's the most common and any requests for more support we can handle later. If teams need to use other components, they won't be blocked as it's also currently an open slot.
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 pretty good! I made a few notes below. Also useOverflowItems
needs tests, and there's a few annotations where certain lines of PageHeader aren't covered by tests either.
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.
Some longer-form docs would be good as well to explain tags
and how they're handled being inserted between tablist and tabpanels. This could be in PageHeader.mdx
I'm seeing this issue too ^^
@kingtraceyj I think the collapse button slot will be added later when we work on the general collapsing functionality |
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.
it looks like the focus state on the last tag is cut off for some reason:
i was also looking through some page header bugs from the old version, and this one popped up about adding a border so that when the header scrolls, it does not blend in to the background. Let's add a $border-subtle
as the bottom border. i'm working with @annawen1 on the border color layer for the breadcrumb bar and they should be the same—either -00
or -01
Closes #19249
Closes #19021
Added tags with overflow handling to PageHeaderTabBar component
Changelog
New
Changed
Testing / Reviewing
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
Wrote passing tests that cover this changeAddressed any impact on accessibility (a11y)More details can be found in the pull request guide