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

Adapt tree list component for content CMS #5270

Merged
merged 34 commits into from
Sep 10, 2024
Merged

Adapt tree list component for content CMS #5270

merged 34 commits into from
Sep 10, 2024

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Aug 2, 2024

Done

  • Added theme support to the list tree component
    • Changed the icon used to indicate expand/collapse state to chevrons
    • Removed the nesting lines as they would conflict with the chevrons
  • Tweaked / cleaned up component padding
  • Added is-active styling and scripting to allow the component to behave more like a navigation component
  • Added an "expanded" version of the list tree example so that the expanded list tree is covered by visual tests
  • Updated the component markup slightly - we were previously mixing interactive (button) elements with noninteractive (list item) elements for the interactive parts of the list. So, we've added a.p-list-tree__link inside of li.p-list-tree__item, which improves keyboard accessibility.

Fixes #5269, WD-11827, WD-13882

QA

  • Open list tree demo
    • Verify it looks correct on all themes. Make sure to try out expanding and collapsing the list.
    • Verify that clicking any node causes the node text to become link-colored.
    • Verify that hovering any node causes it to highlight like a link.
    • Use your keyboard to navigate the tree, and verify that each node can be targeted, and the groupings can be expanded/collapsed. The whole tree should be accessible.
  • Open expanded list tree demo and verify it behaves the same as the default demo, but the entire tree is expanded by default.
  • Open legacy list tree demo and verify that it has the new chevron styling and list spacing, but does not have the new active state highlighting or improved A11y. The legacy component should still work as expected. and be dark-theme friendly.
  • Review combined list tree demo.
  • Review updated tree list docs
  • Review 4.16.0 release notes
  • Review building with Vanilla page and ensure the list trees there are also updated

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

Before:
Screenshot 2024-08-06 at 1 37 17 PM

After:
Screenshot 2024-08-30 at 12 52 15 PM

Sorry, something went wrong.

@webteam-app
Copy link

@jmuzina jmuzina changed the title Theme tree list Add theme support for the list tree component Aug 2, 2024
@lyubomir-popov
Copy link
Contributor

looks good, just a couple of notes:

  • the chevron's bounding box should align with the preceding items, the way this red chevron does - so increase the indent a little (the sidenav already does this I think)

image

active should appear just once, on the leaf node corresponding to the page view displayed in a hypothetical adjacent panel controlled by this componoent. I suggest bold as the simplest thing, that doesn't get in theway of indents

@jmuzina
Copy link
Member Author

jmuzina commented Aug 2, 2024

active should appear just once, on the leaf node corresponding to the page view displayed in a hypothetical adjacent panel controlled by this component.

@lyubomir-popov I'm not certain about this - doesn't this change this component to function more like side navigation, rather than as a list? Maybe we're starting to muddy this component's purpose. My best UX understanding here is that the active state is there to help the user trace through the open tree structure, not to indicate a specific active node.

I'm happy to implement it, just want to be sure I have a clear understanding of this component's purpose.

@lyubomir-popov
Copy link
Contributor

spec should answer https://www.w3.org/WAI/ARIA/apg/patterns/treeview/

@jmuzina
Copy link
Member Author

jmuzina commented Aug 6, 2024

See summary of meeting with @akbarkz on sites' use case here.

@jmuzina jmuzina added the Review: Percy needed This PR needs a review of Percy for visual regressions label Aug 6, 2024
@jmuzina jmuzina force-pushed the theme-tree-list branch 2 times, most recently from ae29447 to 57dfe63 Compare August 6, 2024 17:28
@jmuzina jmuzina removed the Review: Percy needed This PR needs a review of Percy for visual regressions label Aug 6, 2024
@jmuzina
Copy link
Member Author

jmuzina commented Aug 6, 2024

@lyubomir-popov Updated - have a look again please! Be sure to check on small and large screens (indentation changes to align with side navigation)

@jmuzina jmuzina marked this pull request as ready for review August 7, 2024 14:12
Copy link
Contributor

@pastelcyborg pastelcyborg left a comment

Choose a reason for hiding this comment

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

High-level comment: there's a minor metrics thing going on here where the left padding on list tree items isn't sufficient to actually separate the chevron area from the text; this could probably be smoothed out to make the metrics of everything more predictable:

image

scss/_patterns_list-tree.scss Show resolved Hide resolved
scss/_patterns_list-tree.scss Outdated Show resolved Hide resolved
scss/_patterns_list-tree.scss Outdated Show resolved Hide resolved
scss/_patterns_list-tree.scss Outdated Show resolved Hide resolved
@jmuzina jmuzina marked this pull request as draft August 14, 2024 17:14
@jmuzina jmuzina self-assigned this Aug 15, 2024
@jmuzina
Copy link
Member Author

jmuzina commented Aug 15, 2024

High-level comment: there's a minor metrics thing going on here where the left padding on list tree items isn't sufficient to actually separate the chevron area from the text; this could probably be smoothed out to make the metrics of everything more predictable:

@pastelcyborg I've adjusted the margin slightly to eliminate this issue:
image

@jmuzina jmuzina changed the title Add theme support for the list tree component Adapt tree list component for content CMS Aug 15, 2024
@lyubomir-popov
Copy link
Contributor

I still see multiple parents with active states:
image

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
…nd metrics concerns

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
…y (help with maas ui issue)
Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@bartaz
Copy link
Member

bartaz commented Sep 6, 2024

There may have been some hiccup with Percy, as it seems it didn't run?

@jmuzina jmuzina added the Review: Percy needed This PR needs a review of Percy for visual regressions label Sep 6, 2024
@jmuzina
Copy link
Member Author

jmuzina commented Sep 6, 2024

There may have been some hiccup with Percy, as it seems it didn't run?

@bartaz
It was intentionally skipped to save Percy screenshots as I foresaw more changes coming - it's been run and the results are now available

@bartaz bartaz added Review: Percy +1 and removed Review: Percy needed This PR needs a review of Percy for visual regressions labels Sep 6, 2024
@jmuzina
Copy link
Member Author

jmuzina commented Sep 6, 2024

Hi @akbarkz - this is nearing merge, have a look at dark list tree and docs and let me know if you have any thoughts / concerns :)

@jmuzina jmuzina merged commit 4518539 into main Sep 10, 2024
12 checks passed
@jmuzina jmuzina deleted the theme-tree-list branch September 10, 2024 12:54
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.

Theme tree list
5 participants