feat(#3152): update Header component for V2 #3184
Conversation
twjeffery
force-pushed
the
v2-header-update
branch
from
ffe44f6 to
f9c9fea
Compare
twjeffery
changed the base branch from
v2-2998-coded-component-updates
to
dev
twjeffery
force-pushed
the
v2-header-update
branch
from
f9c9fea to
40406dd
Compare
bdfranck
left a comment
bdfranck
left a comment
There was a problem hiding this comment.
I tested version 1 of the header...
- ✅ It looks and works like the current header
- ✅ I don't see any console errors
Regarding all the new code, @chrisolsen may have some thoughts on the following questions:
- Should the new utility menu and overflow features stay in the
AppHeaderor move to new components? - Are there any established patterns for communicating between the
AppHeaderandAppHeaderMenuthat Tom could leverage?
The rest looks good to me! 👍 I just have a couple suggestions.
twjeffery
force-pushed
the
v2-header-update
branch
2 times, most recently
from
0bcd7ed to
0ef4c14
Compare
|
Thanks for the review @bdfranck I've updated the PR with the following changes so far:
|
bdfranck
left a comment
bdfranck
left a comment
There was a problem hiding this comment.
I looked at the latest changes...
- ✅ The Menu uses a version property
- ✅ The Navigation is a separate component
- ✅ There are no console logs
Looks good to me! 👍
| if (_slotParentEl) { | ||
| // Store observer for cleanup in onDestroy | ||
| _observer = new MutationObserver(() => { | ||
| checkForCurrentLink(); | ||
| }); | ||
| // Observe the slot parent for changes to descendants | ||
| _observer.observe(_slotParentEl, { | ||
| attributes: true, | ||
| attributeFilter: ['class'], | ||
| subtree: true | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
We already attach a click event to handle when menu items are clicked, although it currently just closes the menu, but it seems that all the logic within this mutation observer could easily be done within that existing event handler.
There was a problem hiding this comment.
Good call. Removed the MutationObserver.
| function checkForCurrentLink() { | ||
| if (!_slotParentEl) return; | ||
| const slotChildren = getSlottedChildren(_slotParentEl); | ||
| if (slotChildren.length === 0) return; | ||
| const links = slotChildren.filter((el) => el.tagName === "A"); | ||
| const hasCurrentLink = links.some((link) => link.classList.contains("current")); | ||
| _hasCurrentLink = hasCurrentLink; | ||
| } | ||
There was a problem hiding this comment.
I am not sure why this is necessary. Since it is this component that is performs the setting of the current css class, and when it does it sets a flag indicating that the current link has been set, so we already know if there is a current link.
There was a problem hiding this comment.
You're right. Removed this function.
twjeffery
force-pushed
the
v2-header-update
branch
from
0ef4c14 to
21164e8
Compare
twjeffery
force-pushed
the
v2-header-update
branch
from
5e371ad to
21164e8
Compare
| onDestroy(() => { | ||
| // Clean up popover event listeners | ||
| if (_popoverEl) { | ||
| _popoverEl.removeEventListener("_close", closeMenu); | ||
| _popoverEl.removeEventListener("_open", openMenu); | ||
| } | ||
|
|
||
| // Clean up slot parent event listener | ||
| if (_slotParentEl) { | ||
| _slotParentEl.removeEventListener("click", closeMenu); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
This isn't needed, since the all event handlers that are attached internally will be removed when the web component is removed from the dom. Having them doesn't break anything, but it does increase the final build size.
The cases where this should be done, which I am still not 100% sure is needed, is if you are attaching event to a higher up DOM node like the document.body, or something of the like.
There was a problem hiding this comment.
Good point. Removed the onDestroy block
twjeffery
force-pushed
the
v2-header-update
branch
from
21164e8 to
58e10d3
Compare
Summary
Implements V2 app-header component with responsive navigation and utilities (#3152).
Features Implemented
V2 Structure
banner,navigation,utilities,phase"1" | "2"validationResponsive Features
AppHeaderMenu Updates
Technical Implementation
.v2scoping pattern for all V2-specific stylesonDestroyto prevent memory leaksRelated
Testing
Playground page: https://github.com/twjeffery/goa-v2-component-playground/blob/main/src/pages/HeaderPage.svelte
V2
V1