fix(PageToggle): resolved sidebar not collapsing correctly #28
Conversation
|
FYI I'm going to push an update for the alternative method that we had gone over last Friday. Per Coker that alternative method would be the more correct way -- we can tweak things further as a followup once patternfly/patternfly#7377 goes in (this core work would mean not having to apply the classes on the astro island and just having PageSidebar as a child of that island) |
thatblindgeye
force-pushed
the
sidebarBug
branch
from
da9cac7 to
c1acd79
Compare
src/components/PageToggle.tsx
Outdated
| sideBarIsland.classList.add('pf-v6-c-page__sidebar', 'pf-m-expanded') | ||
| sideBarIsland.classList.add( | ||
| 'pf-v6-c-page__sidebar', | ||
| $isNavOpen ? 'pf-m-expanded' : 'pf-m-collapsed', |
thatblindgeye
force-pushed
the
sidebarBug
branch
from
69e4fce to
eb9296c
Compare
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LGTM!
Not a blocker or anything but the hard-coded classes will need manual updates with new PF versions (pf-v6-... to pf-v7-...). At least for those (and probably the modifiers, too, but not as big of a deal) I wonder if we should import '@patternfly/react-styles/css/components/Page/page' so we can use styles.pageSidebar instead of pf-v6-c-page__sidebar
dlabaj
left a comment
dlabaj
left a comment
There was a problem hiding this comment.
Looks like there's a warning for applySidebarStylesToIsland in the hook.
thatblindgeye
force-pushed
the
sidebarBug
branch
from
71ccaa3 to
4365970
Compare
src/components/PageToggle.tsx
Outdated
| function applySidebarStylesToIsland() { | ||
| const isClientSide = typeof window !== 'undefined' | ||
| const sideBarIsland = | ||
| document.getElementById('page-sidebar-body')?.parentElement | ||
|
|
||
| if (!isClientSide || !sideBarIsland) { | ||
| return | ||
| } | ||
|
|
||
| if (!sideBarIsland.classList.contains(styles.pageSidebar)) { | ||
| sideBarIsland.classList.add( | ||
| styles.pageSidebar, | ||
| $isNavOpen ? styles.modifiers.expanded : styles.modifiers.collapsed, | ||
| ) | ||
| } else { | ||
| sideBarIsland.classList.toggle(styles.modifiers.expanded) | ||
| sideBarIsland.classList.toggle(styles.modifiers.collapsed) | ||
| } | ||
| sideBarIsland.setAttribute('aria-hidden', `${!$isNavOpen}`) | ||
| } | ||
|
|
||
| applySidebarStylesToIsland() |
There was a problem hiding this comment.
🤔 is there even much reason to wrap all this in a function and effectively use it as a named IIFE? Seems like we could just have this logic straight in the effect
thatblindgeye
force-pushed
the
sidebarBug
branch
from
4365970 to
d32e1d2
Compare
|
🎉 This PR is included in version 1.0.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
Closes #26