-
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
Try/consolidate headers #5145
Try/consolidate headers #5145
Conversation
52de704
to
f884791
Compare
I THINK this is ready for a solid shake-down. Template Part AlternativesAll header options (with the exception of Skatepark's) have been rolled up into Blockbase as alternate header template parts. The templates in the child themes have been changed to load those alternate templates by default. 'header-linear' is Quadrat's header used by Geologist and Zoologist. 'header-arbutus' is uh... creatively named and only used by Arbututs. 'header-centered' is Seedlet-Block's and 'header-minimal' is Videomaker's. There is MINIMAL (but still some) CSS remaining for the header. It's responsible for modifying the layout and position of the site logo when in 'mobile' view as well as stacking title/tag in mobile view for the 'header-linear' Skatepark is the exception to this. The Theme's header template and the CSS that went along with it were too large and too specific to be able to be refactored easily. Instead the header is still offered as an alternate template (header-ollie), but it's provided by Skatepark rather than Blockbase. This COULD be refactored to simplify the header and extract the CSS making it available for other themes but not for this pass. Header SpacingAll of the template header templates include a new "header-spacer" template. The default template provided by Blockbase is simply a 50px spacer block. Adjusting the spacer in that template part changes the gap across the site. This has the benefits of a css variable in utility (define once, use many places) but is conveniently able to be modified by the user. Where necessary themes that defined that gap in page templates were refactored to instead expect that space in the header. Full Width Background OptionsEach of the headers were (if necessary) refactored to allow a full-width group to have a background color. This was done with standard block layout techniques rather than CSS tricks to ensure that users of the FSE are able to customize the header completely. Site Logo SizingCSS dictating the size of the theme's logo has been removed and replaced with block attributes. Unfortunately our themes have been dictating a HEIGHT while only a WIDTH option is available on the site-logo block. Get Rid of Navigation Block StyleThe alternate block style for navigation that cleans up the responsive menu was removed and is just styling provided by blockbase by default. |
@@ -0,0 +1,27 @@ | |||
<!-- wp:group {"className":"gapless-group","layout":{"inherit":"true"}} --> |
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 think we can achieve the differences in this file using theme.json
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.
In general we want a gap between items, we only want SOME groups to be "gapless". I'm not sure how that could be achieved in theme.json.
Alternately we could just leave the gap and adjust the size of the spacer to be it's current size minus the gap size. The only problem with that was that it looked a little strange as an adjusting mechanism with the gap there, and also there are a couple of themes where the "header gap" is smaller than the "block gap" and the design target couldn't be achieved. If it's OK making those gaps slightly larger than they are now then we could do away with the concept entirely.
I noticed a slight issue in the Videomaker header (it was on trunk too) so I've used the same classes in the header-minimal template to achieve the design intention. |
I noticed that the navigation was off in Quadrat/zoologist/Geologist, so I pushed a fix for that too. |
I made some simplifications to the arbutus header to make it more inline with the other themes. Unfortunately there's no way to specify a block style in theme.json so it will need to be a different template for now. I've renamed it to |
I think this looks good to merge now, but I'd like another review from someone... |
The changes were put into those themes rather than blockbase. I believe those should go in Blockbase so that anything that header option gets the navigation fixes. Also the responsive menus in all themes seem a little wonkey, especially when there are social icons. |
The navigation for these themes is right aligned, not left, so there does need to be something different in the theme somehow. I think the navigation needs greatly simplifying, but I think that needs to be a followup PR as this one is so big already. My aim was just to fix the visual regression so we could ship this :) |
Also those styles are now a part of Blockbase assigned to the The same has been done for videomaker. |
This should be visually correct now and accessible to any Blockbase theme. It could still definitely be simplified. What's been refactored into Blockbase is pretty much whole-hog what came from Quadrat, but it's not too heavy. I think it could be fine as-is until Gutenberg better offers some kind of responsive styling options. I think this is ready for (one more?) look? |
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.
LGTM
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.
Bring it in and we can iterate on Skatepark later on
@Automattic/flow-patrol-manage these changes will need to happen to other Blockbase children... |
…n and close buttons are in alignment.
fdd3c44
to
c5745ae
Compare
What I would try is removing the header template from those themes so it falls back to the default Blockbase one. Blockbase supplies a variety of different header templates now, so you could choose one that you need. If these themes need a different header from the one supplied then you'll need to add the |
Thanks for the tip! I tried the Blockbase headers and the linear one sounds like a good fit, but the spacer at the bottom really throws off the balance (in Byrne and Kerr at least). So I'm guessing I'm still going to need a custom header - unless there is another way? |
If you copy the template part called |
Yeah, I've thought about that too, but I do want zero, and it feels slightly hacky? But happy to do it if it's the right way :) |
You can do it either way, I think both are fine :) |
If you want it zero you could just include an empty header-spacer.html template (like Arbutus). That way no spacer will be added there. Adjusting the space by way of the header-spacer template ensures that the same amount of space (none) will be used should the user switch to another header option. If you make a custom header instead then if the user switches to another style then the default amount of space introduced by Blockbase will be used instead. |
Thanks for the tip @pbking - this is what I'm going to do! |
Empty header spacer causes UX issue, where "plus button" is always visible in the editor: #5236 |
Please, let's remove this concept of a "header spacer template" altogether and look for a different solution. This is super confusing as a user. |
Started from #5120 and encorporating changes and discussion from #5121.
The task of this change is to consolidate header styles designed for child themes into Blockbase which child themes can use as header/footer options. (Or maybe introduce those options in the children, regardless they should be alternative template parts, not replacements).
Closes #5087 #5119 #5066 #5166