react icon indicating copy to clipboard operation
react copied to clipboard

`TreeView`: Align tree item toggle and visual icons to top of item

Open iansan5653 opened this issue 1 year ago • 10 comments

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 TreeView chevron 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)

iansan5653 avatar May 08 '24 16:05 iansan5653

🦋 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

changeset-bot[bot] avatar May 08 '24 16:05 changeset-bot[bot]

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% 🔽)

github-actions[bot] avatar May 08 '24 16:05 github-actions[bot]

@iansan5653 - can we add a story that demonstrates this change? Kind of like this... Screenshot 2024-05-09 at 1 27 01 PM

Alternatively, we could update all existing stories to have an expandable node that has 2 lines of content. That seems unnecessary though...

mperrotti avatar May 09 '24 17:05 mperrotti

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.

github-actions[bot] avatar May 09 '24 17:05 github-actions[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.

image

iansan5653 avatar May 09 '24 18:05 iansan5653

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.

github-actions[bot] avatar May 09 '24 18:05 github-actions[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.

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?

broccolinisoup avatar May 13 '24 07:05 broccolinisoup

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:

iansan5653 avatar May 13 '24 15:05 iansan5653

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.

Storybook Preview

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
A file tree with one tree item highlighted A file tree with one tree item highlighted A overalyed comparison of the before and after file trees with red and yellow colors to indicate differences

JelloBagel avatar May 16 '24 19:05 JelloBagel

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.

github-actions[bot] avatar May 16 '24 23:05 github-actions[bot]