clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-7231] Product Switcher within navigation sidebar

Open nick-livefront opened this issue 1 year ago • 8 comments

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Following the same logic of the current Product Switcher in the upper right corner, available products and/or additional products should show at the bottom of the navigation sidebar.

Code changes

  • product-switcher.service.ts:
    • Moved the product switcher logic from product-switcher-content.component.ts into a service so it can be shared with the existing product switcher and the new one that exists in the navigation UI.
    • I did not rename the bento & other properties but I can if there is a desire for a more general term than bento
    • Added navigationUIDetails to accommodate different verbiage between the navigation product switcher and the current one.
  • navigation-switcher.component.ts:
    • Component to display product offerings vertically within the navigation sidebar.
    • The active product is always hidden
  • toggle-width.component.ts
    • This was absolutely positioned before this change because it was the only item at the bottom of the navigation, and probably because it is only shown in the dev environments. I removed the absolutely positioning because it will always display below the product switcher.
  • tailwind.config.base.js
    • Added xxs font size to match the designs for the "switch" hover text
  • Added product switcher to each navigation instance (if I missed one, let me know!). Also made the nav element a flex container so the product switcher can be pushed to the bottom of the nav.
    • secrets-manager/layout/navigation.component.html
    • organizations/layouts/organization-layout.component.html
    • layouts/user-layout.component.html
    • admin-console/providers/providers-layout.component.html

Open question

  • There is a PaymentMethodWarningsComponent that exists below my implementation of the product switcher. I wasn't able to get this to show by manipulating the code in a reasonable amount of time. Will this cause an overlap of sorts?

Screenshots

More from Bitwarden Available Products
Screen Shot 2024-04-18 at 10 52 26 AM Screen Shot 2024-04-18 at 11 14 47 AM

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

nick-livefront avatar Apr 18 '24 16:04 nick-livefront

Codecov Report

Attention: Patch coverage is 44.95413% with 60 lines in your changes are missing coverage. Please review.

Project coverage is 27.83%. Comparing base (b4f4818) to head (8d04566).

Files Patch % Lines
...navigation-switcher/navigation-switcher.stories.ts 0.00% 33 Missing :warning:
...youts/product-switcher/product-switcher.stories.ts 0.00% 8 Missing :warning:
...roduct-switcher/shared/product-switcher.service.ts 84.09% 0 Missing and 7 partials :warning:
...vigation-switcher/navigation-switcher.component.ts 70.00% 0 Missing and 3 partials :warning:
...bs/components/src/navigation/nav-item.component.ts 0.00% 3 Missing :warning:
...uct-switcher/product-switcher-content.component.ts 33.33% 1 Missing and 1 partial :warning:
libs/components/src/navigation/nav-item.stories.ts 0.00% 2 Missing :warning:
...in-console/providers/providers-layout.component.ts 0.00% 1 Missing :warning:
...eb/src/app/secrets-manager/layout/layout.module.ts 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8810      +/-   ##
==========================================
+ Coverage   27.79%   27.83%   +0.03%     
==========================================
  Files        2422     2425       +3     
  Lines       70233    70296      +63     
  Branches    13096    13099       +3     
==========================================
+ Hits        19521    19566      +45     
- Misses      49192    49202      +10     
- Partials     1520     1528       +8     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 18 '24 16:04 codecov[bot]

Logo Checkmarx One – Scan Summary & Details7fefb693-2245-42ad-8c27-b80bc638ad16

No New Or Fixed Issues Found

github-actions[bot] avatar Apr 18 '24 16:04 github-actions[bot]

~~Converting to draft, I realized I missed some acceptance criteria. I need to add some reference events.~~

Edit: reference events are being omitted from the AC

nick-livefront avatar Apr 18 '24 19:04 nick-livefront

Even though this component is rendered by the sidebar, it actually contains a bit-banner which is positioned at the top of the page.

@eliykat Thank you for validating and the clarification!

Can you please add a Storybook story for the navigation product switcher? See the existing stories for the current product switcher and the layout, for reference.

Yes! I added new stories for the navigation switcher and fixed the existing product switcher stories which I broke with the added service.

ad8bb93

nick-livefront avatar Apr 25 '24 15:04 nick-livefront

@bitwarden/team-platform-dev should own the layout components, see #8814. I've requested their review, please treat them as a code owner (i.e. don't merge without their approval).

eliykat avatar Apr 25 '24 21:04 eliykat

Hey @nick-livefront,

Sorry for the delay. I've discussed the UX with design and we decided to remove the Switch text completely. This should resolve the concerns about it not being clickable. We also discussed some concerns about items changing order as you change product, and the desire is to keep the active product in the nav with a active state similar to general nav items.

Does this seem reasonable to you?

I also had a chat with @willmartian about the duplicated styling and we decided it's fine as it is. There are some future tasks to change how the nav works which would resolve this.

Hinton avatar Apr 30 '24 17:04 Hinton

Sorry for the delay. I've discussed the UX with design and we decided to remove the Switch text completely. This should resolve the concerns about it not being clickable. We also discussed some concerns about items changing order as you change product, and the desire is to keep the active product in the nav with a active state similar to general nav items.

Does this seem reasonable to you?

@Hinton I appreciate you tracking down these answers and they both make sense to me! I'll also note them in the ticket.

  • Removed "switch" text fc39266
  • Allow for NavItemComponent to show active styles ac9ca1c

nick-livefront avatar Apr 30 '24 18:04 nick-livefront

@Hinton @eliykat @LRNcardozoWDF @Thomas-Avery

I had 3 defects come back, I added each as a commit here. Let me know if an alternate process is preferred!

PM-7899 - c78490b PM-7951 - 0b06f42 PM-7984 - a12b95c

nick-livefront avatar May 07 '24 01:05 nick-livefront

@Hinton @eliykat @LRNcardozoWDF @Thomas-Avery

I had another defect come back around but to solve I had to alter a component library component, so I pulled that into a separate PR request: ~~https://github.com/bitwarden/clients/pull/8810~~ https://github.com/bitwarden/clients/pull/9128

nick-livefront avatar May 10 '24 14:05 nick-livefront

I assume you mean https://github.com/bitwarden/clients/pull/9128? :)

eliykat avatar May 12 '24 22:05 eliykat

I assume you mean #9128? :)

Yes I do! Thank you 🙇

nick-livefront avatar May 13 '24 01:05 nick-livefront

@Hinton @eliykat @LRNcardozoWDF @Thomas-Avery

https://github.com/bitwarden/clients/pull/9128 merged into this branch! Ready for review, I appreciate the patience 🙌

nick-livefront avatar May 14 '24 15:05 nick-livefront

@nick-livefront @jtouchstonebw mentioned in chromatic that spacing between icon and text looks a bit smaller than in general.

Hinton avatar May 14 '24 16:05 Hinton

@nick-livefront @jtouchstonebw mentioned in chromatic that spacing between icon and text looks a bit smaller than in general.

@Hinton @jtouchstonebw I missed some margin on the icons in that section, 8d04566 adds the margin to match other icons in the navigation menu

nick-livefront avatar May 14 '24 19:05 nick-livefront