feat: add toggle side option for simplified subpage header#2695
feat: add toggle side option for simplified subpage header#2695faisalahammad wants to merge 5 commits into
Conversation
laurelfulford
left a comment
There was a problem hiding this comment.
Thanks for tackling this one, @faisalahammad! The code overall looks solid -- I added one small bit of feedback but it's minor.
Unfortunately the header in this theme has turned into a beast with the different options and settings -- they all kind of clash together! I ran into a few issues when combining settings but to be honest, this is the kind of stuff I run into when I make changes to the heading, too!
- When the logo is centred (Header Settings > Appearance > Center Logo), the logo, search, and menu toggle are oddly spaced out:
- When a post's featured image is set to the 'beside' position and the logo is centered, everything is kind of grouped together:
On top of the above there's an option to also show the Mobile Call-To-Action when this header is enabled. It doesn't seem to introduce new issues on top of the above, but it changes the alignment a bit so it could be worth testing with as changes are made.
The header is kind of messy after years of features being layered into it! For some context around the feature request, it's an older one that didn't get a lot of traction, but that we left open in case it got more requests in the future. It's a discrete addition (all the changes just affect when the setting's on!) so I think it'd be cool to add, but I also get if the header layout spaghetti is a lot to navigate 🙂
Just let me know if you have any questions about this!
|
Hi @laurelfulford, I've pushed some updates to clean up the code and address the layout quirks you mentioned regarding the centered logo and "beside" featured images:
Ready for another pass when you have a moment! Screenshots:
.
|
laurelfulford
left a comment
There was a problem hiding this comment.
Thanks for the updates, @faisalahammad, and apologies for the delay.
The updates are looking good! There's just one more thing to factor into the styles I think. I made a passing reference to being able to enable the Mobile Call to Action in the simplified header in my previous review, but I didn't explain how to do it. I think these changes just need to be tested with that enabled to make sure it's landing in the right spot.
To enable it:
- Navigate to Customizer > Header Settings > Mobile Call-To-Action
- Check the box to 'Show Mobile CTA'
- Populate the Button URL field - otherwise the button won't show. Just
#totally works here. - At the bottom, check 'Show Mobile CTA in Simplified Subpage header'
With this enabled on this PR, there are some display issues both with the new toggle aligned right option, and the default left alignment.
Standard featured image placements
Settings: toggle left, logo left - button is floating

Settings: toggle left, logo centred - looks good:

Settings: toggle right, logo left - button's floating

Settings: toggle right, logo centred - button's overlapping

'Beside' featured image placements
Settings: toggle left, logo left - button's floating

Settings: toggle left, logo centred - logo's not actually centred

Settings: toggle right, logo left - button's floating

Settings: toggle right, logo centred - button's overlapping

Typically the button's aligned right, like these screenshots from trunk. I think it'd be fine to match this placement for both alignments if it's easiest!
Settings: toggle left, logo left, featured image standard:

Settings: toggle left, logo centred, featured image standard:

Settings: toggle left, logo left, featured image beside:

Settings: toggle left, logo centred, featured image beside:

Just let me know if you have any questions about this! The header's pretty complex, so making changes like this always involves a lot of nitpicky testing 😅
faisalahammad
left a comment
There was a problem hiding this comment.
Addressed Mobile CTA layout issues
Moved the Mobile CTA (newspack_mobile_cta()) and the mobile menu toggle button inside .subpage-right-wrapper so they participate in the same flex flow as the search and right-aligned toggle.
Changes
header.php — Restructured the flex children:
- Mobile CTA and mobile menu toggle are now inside
.subpage-right-wrapper - CTA comes first in the wrapper (visually left-most in right group)
- Order: CTA → mobile-menu-toggle → search → right-toggle
_site-header.scss — Updated .subpage-right-wrapper:
- Added
gapfor consistent spacing between items - Added
flex: noneon> *to prevent items from shrinking - Reset
margin-left: 0on.mb-ctaand.mobile-menu-toggleto override base styles that assume they're direct flex children of.wrapper
_menu-mobile-navigation.scss — Removed obsolete CTA positioning:
- Removed
h-llblock:margin-left: autoon.mb-ctaandmargin-lefton.header-search-contain— no longer needed since CTA is inside the wrapper which already hasmargin-left: auto - Removed
h-clblock:position: absolute; right: 2.25remon.mb-cta— no longer needed since CTA flows naturally in the flex wrapper - Kept the
h-ll.site-branding { margin-right: 0 }rule
How this fixes the reported issues
| Scenario | Before | After |
|---|---|---|
| Toggle left, logo left, CTA on | CTA floating outside wrapper | CTA inside wrapper, aligned right |
| Toggle right, logo left, CTA on | CTA floating | CTA in wrapper flex flow |
| Toggle left/right, logo centered, CTA on | CTA overlapping/absolute | CTA in flex wrapper, no overlap |
| Beside featured image + CTA | Logo not centered | CTA in wrapper, flex: none prevents squish |
Lint
- PHP: clean
- SCSS: clean
- JS: pre-existing ESLint config issue (unrelated)
…positioning Moved Mobile CTA and mobile menu toggle inside .subpage-right-wrapper so all right-side elements share one flex container. Added gap for consistent spacing, flex:none to prevent shrinking, and removed obsolete absolute/floating CTA positioning for h-ll and h-cl contexts. Fixes CTA floating/overlapping in all toggle+logo+CTA combinations reported in review.
There was a problem hiding this comment.
Thanks for taking a swing at this again, @faisalahammad! Apologies again for the round of changes - it's just showing how complex the header's become over the years as we've layered in options.
Given that, this feature might not be worth the risk to land 😕 Our priority with the theme at this point is stability of current features over adding new ones -- this isn't a reflection of your work, but more of the fragility of the underlying code and how each change can affect numerous things. Happy to hear if you see a path through the CSS issues, but wanted to flag where we're at! I've added my notes below:
All featured image placements
On this branch - the Mobile CTA and hamburger menu is now stacked on mobile due to the markup change:
Same settings on trunk:
Logo centred, toggle left, featured image beside
On this branch, the logo is aligned left:
Same settings, on trunk:
Logo centred, toggle right, featured image beside
Similar issue with the right aligned toggle - the logo is aligned left. This one IS kind of a weird combo, but the settings don't match the appearance:
Logo centred, toggle right, mobile CTA
This is more of a nitpick and DEFINITELY an issue the theme has with other settings, but the logo is more noticably offset left when the mobile cta is visible. This could be fixed by using absolute positioning for those right-aligned elements, but it's not a blocker:




Summary
Added a new Customizer setting to independently position the simplified subpage header's hamburger menu toggle on the left or right. This mirrors the existing capability for the desktop slide-out sidebar, ensuring a consistent user experience across different header layouts.
Fixes #1957
Changes
customizer.php
Before:
After:
Why: Reuses the existing sanitization logic and follows the established pattern for sidebar positioning to provide independent control for the subpage header toggle.
header.php
Before:
After:
Why: Dynamically positions the toggle container based on the user's preference, ensuring proper alignment in the header layout.
Testing
Test 1: Alignment and Slide Direction
Result: Works as expected.
Test 2: Independence
Result: Works as expected.