`TreeView`: Align tree item toggle and visual icons to top of item
The vast majority of the time, TreeView items are only one line and the content is truncated to fit. However, they can potentially wrap, as in https://github.com/github/collaboration-workflows-flex/issues/902 (internal issue). In this case, we want the chevron to still remain locked to the top of the item, rather than vertically centered.
This is slightly challenging because the height of items changes depending on the input mode, expanding automatically for touch controls. It wasn't complicated to move the min-height to a CSS variable and calculate a correct padding based on that: min-height / 2 - icon-height.
There is no visual change here for the non-wrapped item.
Changelog
New
Changed
- Always align
TreeViewchevron icon to top of item
Removed
Rollout strategy
- [x] Patch release
- [ ] Minor release
- [ ] Major release; if selected, include a written rollout or migration plan
- [ ] None; if selected, include a brief description as to why
Testing & Reviewing
Merge checklist
- [ ] Added/updated tests
- [ ] Added/updated documentation
- [ ] Added/updated previews (Storybook)
- [ ] Changes are SSR compatible
- [ ] Tested in Chrome
- [ ] Tested in Firefox
- [ ] Tested in Safari
- [ ] Tested in Edge
- [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)
🦋 Changeset detected
Latest commit: 39c1f6e175f4c05c9ee0ad2ed7b942c1511e3f23
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| @primer/react | Patch |
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 📦
| Path | Size |
|---|---|
| packages/react/dist/browser.esm.js | 88.53 KB (+0.04% 🔺) |
| packages/react/dist/browser.umd.js | 88.84 KB (-0.01% 🔽) |
@iansan5653 - can we add a story that demonstrates this change? Kind of like this...
Alternatively, we could update all existing stories to have an expandable node that has 2 lines of content. That seems unnecessary though...
Uh oh! @mperrotti, the image you shared is missing helpful alt text. Check https://github.com/primer/react/pull/4572#issuecomment-2103100406.
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.
Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.
Added a story. Now that I'm looking at it I'm thinking maybe we should top-align the leading and trailing visuals as well? I'm not totally sure how I feel about this approach given the leading/trailing visual appearance... maybe overriding the styling for the specific use case of sub-issues is actually better 🤔 But we don't have any official style overriding support, requiring us to reach in and select PRIVATE classes, which is hacky.
Uh oh! @iansan5653, the image you shared is missing helpful alt text. Check https://github.com/primer/react/pull/4572#issuecomment-2103174185.
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.
Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.
Added a story. Now that I'm looking at it I'm thinking maybe we should top-align the leading and trailing visuals as well? I'm not totally sure how I feel about this approach given the leading/trailing visual appearance... maybe overriding the styling for the specific use case of sub-issues is actually better 🤔 But we don't have any official style overriding support, requiring us to reach in and select
PRIVATEclasses, which is hacky.
It makes sense o me to align all to the top - not the same case but we took a similar approach for pageheader as well that we align all visuals to the top when the title is multiline
@mperrotti thoughts?
It makes sense o me to align all to the top - not the same case but we took a similar approach for pageheader as well that we align all visuals to the top when the title is multiline
If that's the preferred approach I'd be happy to implement it :+1:
I updated the leading/trailing visual to be top aligned as well: 185b613. I removed the min-height to properly align the visuals with the text but kept the same visual height by adding padding to the top and bottom of the text.
Edit: VRT in CI was failing so I added the update-snapshots label but the difference between the before and after looked nominal locally
| Before | After | Diff |
|---|---|---|
Uh oh! @JelloBagel, the image you shared is missing helpful alt text. Check https://github.com/primer/react/pull/4572#issuecomment-2115984339.
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.
Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.