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

Hds::SideNav - add @isCollapsible and @isMinimized arguments #1630

Merged
merged 35 commits into from Oct 3, 2023

Conversation

alex-ju
Copy link
Member

@alex-ju alex-ju commented Sep 12, 2023

πŸ“Œ Summary

  • Add @isCollapsible (default false) to allow users to collapse/expand the SideNav on any screen size (previously only available on narrow viewports)
  • Expose the @isMinimized (default false) argument to allow applications to preserve/control the minimized state across sessions
  • Visual changes:
    • the collapse/expand button is now using double chevrons
    • the SideNav is smaller when minimized and doesn't include the logo anymore
    • the SideNav has a light border to the right

πŸ› οΈ Detailed description

As a prerequisite of this work, we separated the minimized/not-minimized state from is-deskop/is-mobile styles before introducing the optional isCollapsible argument that determines whether or not to show the expand/collapse button when in desktop mode. On mobile, the collapsible feature is always on, with the exception when isResponsive is explicitly set to false.

Note: To test the work during development we use a dummy demo app; this does not necessarily need to be merged in, but I kept it to ease the review.

πŸ‘‰ Preview current Hds::SideNav in a dummy app

πŸ‘‰ Preview proposed Hds::SideNav with @isCollapsible={{true}} in a dummy app

Ongoing tasks:

  • Review changes in existing applications
  • Gather feedback from stakeholders
  • Update documentation
  • Add keyboard shortcuts to expand/contract the navigation

πŸ“Έ Screenshots

Default

BeforeAfter

01-default-desktop-before

02-default-desktop-after

Screenshot 2023-09-18 at 19 53 34 Screenshot 2023-09-18 at 19 49 51

05-expanded-mobile-before

06-expanded-mobile-after

Collapsible

The following screen captures illustrate the effects of setting @isCollapsible={{true}} in the dummy app, HCP and TFC

Local test in dummy app
Expanded/openCollapsed/closed
07-collapsible-default-2 08-collapsible-collapsed-2
Local test in HCP
sidenav-cloud-ui.mp4
Local test in TFC
sidenav-atlas.mp4

πŸ”— External links

Jira ticket: HDS-2388
Figma file: https://www.figma.com/file/I4gqjXgIvyrX1hBAodx9sC/Collapsible-SideNav-Exploration-%26-Milestones?type=design&node-id=43471-138048&mode=dev


πŸ‘€ Reviewer's checklist:

  • +1 Percy if applicable
  • Confirm that PR has a changelog update via Changesets if needed
  • Confirm that A11y tests have been run locally for this component

πŸ’¬ Please consider using conventional comments when reviewing this PR.

@vercel
Copy link

vercel bot commented Sep 12, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
hds-website βœ… Ready (Inspect) Visit Preview Oct 3, 2023 3:21pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
hds-showcase ⬜️ Ignored (Inspect) Visit Preview Oct 3, 2023 3:21pm

@alex-ju alex-ju changed the title Hds::SideNav - add @isCollapsable and @isMinimized arguments Hds::SideNav - add @isCollapsible and @isMinimized arguments Sep 12, 2023
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

@alex-ju did a first pass and stress-tested the implementation. for now I don't see any blocker, just general comments/suggestions (and a few questions I want to discuss with you, I'll set up a meeting for this)

@didoo
Copy link
Contributor

didoo commented Sep 13, 2023

@alex-ju another thing to discuss: in the RFC I see mentioned that this feature is opt-in, but I don't see it in the code (but maybe I have missed it)

Copy link
Contributor

@jorytindall jorytindall left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-09-15 at 5 12 30 PM

This is looking slick! I'm still waiting for any additional feedback from stakeholders, which I'd hope to wrap by Monday next week. Otherwise, I dropped one initial comment, and noting here that it looks like the scroll bar (or some other element?) is floating a bit on small viewports?

@alex-ju
Copy link
Member Author

alex-ju commented Sep 19, 2023

noting here that it looks like the scroll bar (or some other element?) is floating a bit on small viewports?

@jorytindall, I've not been able to replicate this, and although I've not changed the layout of this component (I suspect if there's an issue with scrolling it's something that we'd see in the current implementation too) I'd be keen to look into this (even if it's part of a separate PR). Would you be able to provide me with the browser/os combination that's causing issues?

@alex-ju alex-ju marked this pull request as ready for review September 19, 2023 14:22
@alex-ju
Copy link
Member Author

alex-ju commented Sep 19, 2023

@didoo I tried to address all the things we discussed in the sync review as well as the comments here, so it's good for another review whenever time allows

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

@alex-ju I haven't checked (yet) the CSS line by line, I intend to do it when it's 100% finalized.
In the meantime I've left a few comments that I hope can help in moving forward the PR to a "ready to merge" state.

.changeset/two-pumas-deny.md Outdated Show resolved Hide resolved
packages/components/addon/components/hds/side-nav/index.js Outdated Show resolved Hide resolved
packages/components/addon/components/hds/side-nav/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

Reviewed latest changes and re-approved

Copy link
Contributor

@jorytindall jorytindall left a comment

Choose a reason for hiding this comment

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

For some reason the preview of the demo app isn't working for me (all I get is a blank page). The only thing I wanted to confirm was the animation/easing between the toggle button and the SideNav and whether that was either fixed or reverted to the same values to solve that separation issue in the animation rendering. Looking through the code I think they are the same, but could be wrong.

Otherwise, everything looks solid to me πŸŽ‰

height: 36px;
padding: 0 4px;
color: var(--token-color-foreground-high-contrast);
background: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to override background styles being inherited from somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

a reset for the <button> element

Copy link
Contributor

@KristinLBradley KristinLBradley left a comment

Choose a reason for hiding this comment

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

Added a few suggested text/punctuation edits.

@MelSumner
Copy link
Contributor

@alex-ju any reason why the tab order is skipping the switchers/selects in the sidenav?

@alex-ju
Copy link
Member Author

alex-ju commented Oct 3, 2023

@alex-ju any reason why the tab order is skipping the switchers/selects in the sidenav?

yes, that's not an actual interactive element (it's a div styled to look like an organization select). this element is part of the demo app work that Cristiano suggested we introduce in a separate PR so we can address it there

Co-authored-by: Kristin Bradley <kristin.bradley@hashicorp.com>
Co-authored-by: Jory Tindall <jory.tindall@hashicorp.com>
@alex-ju alex-ju merged commit 4765799 into main Oct 3, 2023
15 checks passed
@alex-ju alex-ju deleted the alex-ju/collapsable-sidenav branch October 3, 2023 16:45
@hashibot-hds hashibot-hds mentioned this pull request Oct 3, 2023
@alex-ju alex-ju mentioned this pull request Oct 4, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website packages/components packages/tokens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants