Add truncation features to the LabelGroup component #3264
Conversation
…n width using a ref callback
…e IntersectionObserver rootMargin
🦋 Changeset detectedLatest commit: 674274b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
| // We need to manually re-focus the collapse button if we're not showing the full | ||
| // list in an overlay. | ||
| // TODO: get rid of this hack | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Help wanted: I used this setTimeout because we need to wait until state has been updated and the expand button is back in the DOM.
This feels sloppy/hacky, but I haven't been able to figure out a better way. @siddharthkp - we paired on this last week.
…to mp/labelgroup-truncation
|
@broccolinisoup - could you give this another look when you get a chance? |
There was a problem hiding this comment.
Thanks so much @mperrotti
I tested it again with the issues I mentioned before in mind and all working beautifully ✨
Tested with a mouse and keyboard
- Non interactive tokens with auto truncate/expand inline
- Non interactive tokens with auto truncate/expand overlay
- Interactive tokens with auto truncate/expand inline
- Interactive tokens with auto truncate/expand overlay
On Chrome, Safari, Edge and Firefox.
It looks great to me overall. Just tiny styling and focus issue I just discovered but I don't think it is a blocker for this PR. Let me know what you think.
Focus ring on Edge (latest)
Initial focus is wrong on Firefox
Initial focus on Firefox jumps to the element that is just before the +X in this story (components-labelgroup-features--truncate-auto-expand-inline-with-interactive-tokens) I can't add a video sorry - something is wrong with my Kap but let me know if you can't replicate it.
Otherwise, I love these changes and thanks for your patience with my while I was being nitpicky 😁
|
@broccolinisoup - I'm able to reproduce the focus order problem in Firefox. I have no idea why this is happening, but I think we need to fix it before we can merge this :( Kapture.2023-07-27.at.09.45.47.mp4 |
|
I am unable to reproduce this on https://primer-018a39f5ea-13348165.drafts.github.io/storybook/?path=/story/components-labelgroup-features--truncate-auto-with-interactive-tokens. Firefox 115.0.2 on macOS Ventura 13.5 in a private window. One thing I'm wondering: is focus following last click placement? That can affect things. results.mov |
|
Interesting - I'm also on Firefox 115.0.2, but macOS Ventura 13.4.1. I'm going to play with it a little bit more, but I think it might be safe to merge as-is. |
|
I'm not able to reproduce when I load the story in it's own window. I think it's safe to merge 🙂 |
These changes add the following truncation features to the LabelGroup component:
Consumers can choose to show the full list of labels inline or in an Overlay.
Closes https://github.com/github/primer/issues/2006
Screenshots
Truncate after
nchildren and show the full list inlineNote: this screen recording is slightly out of date. Instead of rendering a "—" icon to collapse tokens, we now show a text button that says "Show less"
Kapture.2023-05-08.at.16.57.18.mp4
Truncate to fit parent width and show the full list in an overlay
Kapture.2023-05-08.at.16.58.59.mp4
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.