Skip to content
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

Actionlist tree-view CSS #1803

Merged
merged 34 commits into from
Dec 16, 2021
Merged

Actionlist tree-view CSS #1803

merged 34 commits into from
Dec 16, 2021

Conversation

langermank
Copy link
Contributor

@langermank langermank commented Dec 1, 2021

Overview

ActionList tree-view is a modifier for ActionList: .ActionList--tree. It uses default styles from .ActionList with sensible defaults for a tree-grid or tree-view scenario.

Updating ActionList to use ActionList--tree

element class role attributes
ul ActionList ActionList--tree "tree" none
ul ActionList ActionList--subGroup "group" none
li ActionList-item ActionList-item--hasSubItem "none" if a exists aria-level="" aria-setsize="" aria-posinset=""
li ActionList-item ActionList-item--subItem "none" if a exists aria-level="" aria-setsize="" aria-posinset=""
a ActionList-content "treeitem" none

aria-level: this should be a number (starting at 1) indicating the hierarchical structure of the tree. The easiest way to understand how it works for tree-view is to inspect this story in Storybook. There should be an inline style defining a CSS variable --ActionList-tree-depth: X with the value from aria-level. This is what creates the nested styles.
aria-setsize and aria-posinset are referenced in this tree example so I included them in my story.

aria-setsize: Defines the number of treeitem elements in the set of treeitem elements that are in the same branch and at the same level within the hierarchy.1

aria-posinset: Defines the position of the element within the set of other treeitem elements that are in the same branch and at the same level within the hierarchy.2

This component still requires an official accessibility audit- these are recommendations based on my own research and have not yet been tested

Also note: visual modifier classes shouldn't be needed for tree-view -ActionList-content--visual16 for example can be removed from markup.

Other fixes

  • Changes ActionList-item grid alignment to start for better multi-line support
  • Moves nesting padding logic to ActionList-content (the contents of an item, usually the a href) which allows the hover state to fully extend the container bounds. Scoped to expandable sections only.
  • Adds a prop to allow ActionList-item to have a small font-size instead of defaulting to small for nested groups (nested groups will use this prop in PVC)

Tree view fixes/additions

  • Fix hover state on nested groups
  • Vertical connected lines
  • Truncate option for items
  • CSS var for nested group logic
  • Chevron animation/rotation

Footnotes

  1. Role, Property, State, and Tabindex Attributes

  2. Role, Property, State, and Tabindex Attributes

@changeset-bot
Copy link

changeset-bot bot commented Dec 1, 2021

🦋 Changeset detected

Latest commit: b9ce8c9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css 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

// --ActionList-tree-depth is defined as an inline style referencing the aria-level of each item ie: aria-level="2"
// stylelint-disable-next-line selector-max-specificity, max-nesting-depth, selector-max-compound-selectors, primer/spacing
.ActionList-content {
padding-left: calc(#{$spacer-2} * var(--ActionList-tree-depth));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
padding-left: calc(#{$spacer-2} * var(--ActionList-tree-depth));
padding-left: calc($spacer-2 * var(--ActionList-tree-depth));

Does this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It compiles correctly, but still getting a lint error. This is definitely a tomorrow problem 😂

@simurai
Copy link
Contributor

simurai commented Dec 15, 2021

ActionList tree-view is a modifier for ActionList: .ActionList--tree

Not sure if this came up before, but have we considered adding a tree-view as its own component? Maybe as ActionTree? It has a lot in common with ActionList but could also be specific enough to warrant a separate component? A downside is probably that we would have to repeat a lot of the styles, instead of just overriding/extending.

Or maybe there could be a separate ActionItem component that would be shared. Like:

  • ActionItem only cares about the content inside the item, but not where/how the item is used.
  • ActionList shows ActionItems as a list
  • ActionTree shows ActionItems as a tree view
  • ActionMenu shows ActionItems as a menu that can have nested items

Just trying to think about ways how to avoid making ActionList too complex. I'm also aware that re-organizing it into separate components would be TONS of additional work. Especially if PVC/PRC should adopt that too. So feel free to ignore 🙉 this comment.

/cc @vdepizzol

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. ✨

The only thing I was wondering is if the indentation should be a bit more spacious.

Before After
Screen Shot 2021-12-15 at 12 37 40 Screen Shot 2021-12-15 at 12 37 19
8px 12px

I know 12px doesn't exist in the system, but 8px just feels a bit tight.

@langermank
Copy link
Contributor Author

I know 12px doesn't exist in the system, but 8px just feels a bit tight.

I agree that it feels tight. Originally I had it much more spaced out, but the use case for this component requires a much more condensed design. I thought about adding a prop for condensed with a spacious variant, but I think we should wait to add that if a use case comes up. ActionList CSS architecture can easily support adding more modifiers like that 🎉

@langermank
Copy link
Contributor Author

Not sure if this came up before, but have we considered adding a tree-view as its own component? Maybe as ActionTree? It has a lot in common with ActionList but could also be specific enough to warrant a separate component? A downside is probably that we would have to repeat a lot of the styles, instead of just overriding/extending.

Definitely worth bringing up so thank you! The way you broke it down essentially is how I'm thinking about it. I separated "components" into their own files, but inevitably they still have dependencies between them.

For ActionList--tree specifically, if I had found myself having to override or undo a substantial amount of styles from ActionList that's when I would start to consider this not a modifier and more of a separate component. But I didn't really find that to be the case here. I think for future proofing (you know, as much as we can) its easier to extract CSS into multiple components later vs. keeping multiple components in sync now.

@vdepizzol
Copy link
Contributor

@simurai +1 to @langermank replies here :)) This tree view is initially appearing in sidebars, so horizontal spacing is crucial.

For ActionList--tree specifically, if I had found myself having to override or undo a substantial amount of styles from ActionList that's when I would start to consider this not a modifier and more of a separate component. But I didn't really find that to be the case here. I think for future proofing (you know, as much as we can) its easier to extract CSS into multiple components later vs. keeping multiple components in sync now.

👍 👍

@langermank langermank merged commit c46fe91 into main Dec 16, 2021
@langermank langermank deleted the actionlist-treeview branch December 16, 2021 19:40
@primer-css primer-css mentioned this pull request Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants