-
Notifications
You must be signed in to change notification settings - Fork 361
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
change: [M3-8659] - Incorporate Product Family Groups in Side Nav #11080
change: [M3-8659] - Incorporate Product Family Groups in Side Nav #11080
Conversation
@hana-akamai Happy to address the test failures! I'm guessing we'll need to make changes to some of our utils and a bunch of the tests, so I'll try to take a closer look soon! |
@jdamore-linode this is still a WIP so things are probably going to change, I'll let you know when this is ready for review! |
Coverage Report: ✅ |
@@ -0,0 +1,71 @@ | |||
import { BetaChip } from '@linode/ui'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was more or less moved from PrimaryNav
let updatedCollapsedAccordions; | ||
if (collapsedAccordions.includes(index)) { | ||
updatedCollapsedAccordions = collapsedAccordions.filter( | ||
(accIndex) => accIndex !== index | ||
); | ||
updatePreferences({ | ||
collapsedSideNavProductFamilies: updatedCollapsedAccordions, | ||
}); | ||
setCollapsedAccordions(updatedCollapsedAccordions); | ||
} else { | ||
updatedCollapsedAccordions = [...collapsedAccordions, index]; | ||
updatePreferences({ | ||
collapsedSideNavProductFamilies: updatedCollapsedAccordions, | ||
}); | ||
setCollapsedAccordions(updatedCollapsedAccordions); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ugly, but I figured it was better than using a useEffect
to update preferences
} | ||
|
||
// Wrapper around PrimaryLink that includes the usePrefetchHook. | ||
export const PrefetchPrimaryLink = React.memo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the prefetching code since it's no longer being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the workarounds here! Going forward I think we'll eventually have to refactor or extend ui.nav
's helpers to account for product families in the nav bar:
- Situations where a nav item has the same title as a product family (like in this case with "Monitor")
- Resilience against cases where a nav item can't be found because its product family is collapsed
Happy to have these tests passing with cy.get
in the meantime, thanks again! I'll take a closer look at this and open a ticket for a long term solution soon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was looking really good!
create-volume.smoke.spec.ts
does seem to be failing consistently (CI and locally for me) - might be something going on there, unless this test started being recently flaky outside of this PR - I just hadn't noticed much of that.
packages/manager/src/components/PrimaryNav/PrimaryNav.styles.ts
Outdated
Show resolved
Hide resolved
| Ah, good catch @mjac0bs! @hana-akamai this is just because the test is trying to click the "Storage" tab on the Linode details page, but the new "Storage" nav heading is tripping up the test. Super easy fix to replace the cy.get('main').within(() => {
cy.findByText('Storage').should('be.visible').click();
}); Edit: and confirmed that the other 2 CI failures can be disregarded 👍 |
Thanks @jdamore-linode! Is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall functionality looks good - product family icons are highlighted when on that page, expanded and collapsed state works as expected, transitions look good (one note about the Akamai logo), and the empty state page icons all match up.
Approving pending that one test Joe noted and ideally an adjustment to the logo transition. Thanks for the changes and the follow up ticket for the icons.
packages/manager/src/components/PrimaryNav/PrimaryNav.styles.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/components/PrimaryNav/PrimaryNav.styles.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
Collapsed/expanded sections + collapsed/expanded menu persist across refreshes ✅
Highlighted product family icon based on page you are currently on in Cloud ✅
Code review ✅
@hana-akamai For that test, Cypress is trying to click the "Storage" tab on the Linode details page instead of a nav item, but the new Storage product family heading is causing the test to trip up since it only expects to find one instance of the text "Storage". I used |
Description 📝
More products are getting added to Cloud Manager, so we need to make sure our side navigation menu can accommodate them.
This PR introduces collapsible Product families into the side nav to allow more products to be shown while also giving users the ability to hide/show relevant families. Additionally, individual product icons have been removed in favor of Product family icons
Note: The Monitor product family icon is still in discussion. Create menu dropdown will be updated in M3-8728.
Changes 🔄
Code clean up:
Preview 📷
Screen.Recording.2024-10-28.at.1.38.17.PM.mov
How to test 🧪
Verification steps
(How to verify changes)
As an Author I have considered 🤔
Check all that apply