-
Notifications
You must be signed in to change notification settings - Fork 30
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
Header & Footer: Use core supports for text, background, and border controls #350
Conversation
What about backward compatibility? |
Should be fine, the old style of |
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.
This works (provided your Gutenberg is up to date to support skipBlockSupportAttributes
) and the code makes sense!
I'm not completely familiar with how the header/footer works in places like trac... it's probably worth checking that out before we deploy this.
It's synced from an API endpoint, but good catch, I'll do a bit more sandbox testing. It should be fine since the defaults all still work. |
Okay, works fine with the API endpoint, but it's actually wonky with the classic themes, since the full header output is wrapped in |
Okay, fixed that so now the opening & closing html tags & code are not wrapped in the header/footer tags on classic themes. Do you want to re-review @StevenDufresne? |
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.
Just caught up on the context and tested the code, and things look good 👍
Except for the editor part that:
- When I change the color, it doesn't change accordingly in the editor, but your screenshot shows that it does.
- Also, when I change the header style by modifying the template file, it only reflects on the front end, in the editor, it's still white-on-black.
Perhaps these are the wonky part that you mentioned.
No, the changes are relatively minor. 👍 |
Yeah, I should have been more clear 😅 — I'm not sure why it's not rendering the color in the editor anymore (it was when I took the screenshot in the description, but not now), but I think we can ignore that issue. This won't be edited in the editor anyway.
Can you explain, or share the block code where you're seeing this issue? |
I first replaced the default globalHeader block with
in and in the editor, it was like Its code was
If I switch to another color and then switch back, or if I unselect color
And the front-end would be broken then.
But since you mentioned that this wouldn't be edited in the editor, so I suppose it doesn't matter and can be skipped for now? |
Oh, I see now. I addressed this in the description somewhat; while arbitrary background colors are somewhat supported for the header, if we actually want to implement a new color style, it will need design and custom CSS. I think that's out of scope for this PR, since we don't even know if that will be necessary (if you look at the current i3 in developer, it still uses a black global-header). |
I got it now, it's a situation that's uncertain in a couple of ways. Thank you for taking the extra time to explain it to me 🙂 |
Fixes #338, fixes #341 — This enables the core supports for the text color, background color, and border controls on Global Header and Global Footer, replacing the existing "variations" approach for styling. Rather than continuing to add variations for "expressive" sites, we can allow configuration and tweak the CSS as needed.
Screenshots
The first three screenshots match the current behavior.
In the editor you can see the new UI
Custom colors are supported on the header & footer. The footer handles this fine, since there are only two colors involved. Technically you can change the background color of the header to any color too, but the hover states & dropdowns are not smart enough to match arbitrary colors. Currently I've got this falling back to the grey dropdown when the text is a
charcoal
. But even the blue "current" state clashes. If/when we want new colors for the header, we'll need to add some extra CSS.To test
You technically can try this in the site editor, but the blocks themselves are a little wonky. It's easier to test by editing the block in a template file, for example, try any of the following (swap out
header
/footer
, different colors, etc):Default
<!-- wp:wporg/global-header /-->
Black on white
<!-- wp:wporg/global-header {"backgroundColor":"white","textColor":"charcoal-2"} /-->
Black on a custom color
<!-- wp:wporg/global-footer {"textColor":"charcoal-2","style":{"color":{"background":"#6ec9ae"}}} /-->
Black on white with a border
<!-- wp:wporg/global-header {"textColor":"charcoal-2","backgroundColor":"white","style":{"border":{"bottom":{"color":"#0000001a","width":"1px","style":"solid"}}}} /-->
White on blue (using
style
syntax to ensure back-compat for the sites that are currently using it)<!-- wp:wporg/global-header {"style":"white-on-blue"} /-->
❗ Reviewing note: The changes in
header.php
andfooter.php
look massive, but it's mostly indentation changes because the wrapper element has been moved up toblocks.php
.