Minor Nav Refactor + Fix for Overlapping Dropdown Menus #1160
Conversation
|
I made a little test with the netlify preview. Seems that the sidebar layout is broken due to these changes. Pattern groups cannot be opened. |
Oof. Can't believe I missed that. 🤦♂️ Pushing up a fix! |
That's also a reminder to myself that we need to get some UI regression tests in place (Jest + Nightwatch or perhaps Cypress). |
@JosefBredereck all set with the fix for this |
additionally to add to that list: https://github.com/garris/BackstopJS for visual regressions with simple interactions (click and hover) on elements. |
|
Wouldn't it be a good chance to reactor the pl-nav component to be a lit component? Also I would like to merge this one before #1143 because there are also changes in this file and the conflict will be easier to resolve this way. |
| this.topLevelTriggers.forEach(trigger => { | ||
| if (trigger !== target) { | ||
| trigger.classList.remove('pl-is-active'); | ||
| trigger.nextSibling.classList.remove('pl-is-active'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
If you walk through all trigger, why you need to remove the pl-is-active class from the next sibling?
There was a problem hiding this comment.
This chunk of code was just copied over from what we were already doing in the full Nav cleanup method.
Keep in mind that despite changing how the Nav was previously being rendered, much of the current code here hasn't been fully refactored (hence there's still some cruft to clean up like what you're pointing out here).
| const nonViewAllItems = elem.noViewAll | ||
| ? children.filter(item => item.patternName !== 'View All') | ||
| : children.filter( | ||
| item => | ||
| item.patternName !== 'View All' && !item.patternName.includes(' Docs') | ||
| ); |
There was a problem hiding this comment.
Im not shure where patterns with Docs come from, I cant find anything in the core package regarding this pattern name.
There was a problem hiding this comment.
Ah. This was something I started to play around with (upstream from Pattern Lab) in the Bolt Design System.
Basically it's a yet to be documented feature that lets you replace the generic "View All" template with something a bit more custom (ie. extra info like the component name, description, status, etc).
@JosefBredereck In an ideal world? Maybe. However keep in mind that I'm trying to be pragmatic (and realistic) here and not bite off more work than I know I have time to see things through. This PR isn't meant to be an entire refactor of the Nav (which takes a lot more time and requires extensively more testing as I realized the last time I tried to tackle everything all at once). Rather, this PR is meant to make it a lot easier and more manageable to incrementally refactor the Nav (and migrate over to LitElement). That's precisely why I held back making any additional code changes than the minimum required to get something we can ship very quickly.
Agreed -- this is meant to be super quick to merge down (and should make it super easy to resolve merge conflicts in your other PR). I want to make sure we're careful with #1143 so we don't unintentionally cause regressions downstream. |
|
Ok, if there is nothing to add, then I will approve this change. |
Thanks @JosefBredereck, really appreciate it. |
agreed - i've been reading through this a lot and have thoughts, but want to test it all more first before weighing in. Excited at all the fixes this proposal introduces, but the change to sort and flatt pattern behavior is flexing my working understanding of how the nav should work. |
…efactor Minor Nav Refactor + Fix for Overlapping Dropdown Menus
This PR makes two basic changes to the Nav in Pattern Lab's UIKit:
It starts to tackle the overall Nav refactor I started in UIKit Nav Cleanup #1102 by breaking apart the existing Nav UI into smaller, more manageable pieces, but without any visual or functional changes. This should help make any additional Nav refactor work a lot more palatable.
It fixes the annoying bug of having overlapping dropdown menus (on larger screens while the Nav is in Horizontal mode).