Skip to content
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

Merged
merged 11 commits into from
Mar 17, 2023

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Mar 13, 2023

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.

Variation Screenshot
White on black (default) default
Black on white black-on-white
White on blue white-on-blue
With a border with-borders
Custom background custom-color

In the editor you can see the new UI

in-editor

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.

custom-color-header

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 and footer.php look massive, but it's mostly indentation changes because the wrapper element has been moved up to blocks.php.

@StevenDufresne
Copy link
Contributor

What about backward compatibility?

@ryelle
Copy link
Contributor Author

ryelle commented Mar 14, 2023

What about backward compatibility?

Should be fine, the old style of <!-- wp:wporg/global-header {"style":"white-on-blue"} /--> will still work. This function converts the old style attribute to the new textColor & backgroundColor.

Copy link
Contributor

@StevenDufresne StevenDufresne left a 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.

@ryelle
Copy link
Contributor Author

ryelle commented Mar 15, 2023

I'm not completely familiar with how the header/footer works in places like trac

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.

@ryelle
Copy link
Contributor Author

ryelle commented Mar 15, 2023

Okay, works fine with the API endpoint, but it's actually wonky with the classic themes, since the full header output is wrapped in header tags now, including the <head> code.

@ryelle
Copy link
Contributor Author

ryelle commented Mar 15, 2023

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?

Copy link
Contributor

@renintw renintw left a 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:

  1. When I change the color, it doesn't change accordingly in the editor, but your screenshot shows that it does.
  2. 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.

@StevenDufresne
Copy link
Contributor

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?

No, the changes are relatively minor. 👍

@ryelle
Copy link
Contributor Author

ryelle commented Mar 16, 2023

Perhaps these are the wonky part that you mentioned.

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.

I noticed that editing the global header background color in the editor can cause the class name to change, even if the color is the same. This would make the hover color reset here ineffective.

Can you explain, or share the block code where you're seeing this issue?

@renintw
Copy link
Contributor

renintw commented Mar 16, 2023

Can you explain, or share the block code where you're seeing this issue?

I first replaced the default globalHeader block with

<!-- wp:wporg/global-header {"textColor":"charcoal-2","style":{"color":{"background":"#6ec9ae"}}} /-->

in front-page.html, so the front-end was like:

1

and in the editor, it was like

Screen Shot 2023-03-16 at 11 54 34 PM

Its code was

<!-- wp:wporg/global-header {"style":{"color":{"background":"#6ec9ae"}},"textColor":"charcoal-2"} /-->

If I switch to another color and then switch back, or if I unselect color #6ec9ae from the color palette and then select it again, the code would change to the following:

<!-- wp:wporg/global-header {"backgroundColor":"aquamarine","textColor":"charcoal-2"} /-->

And the front-end would be broken then.

Screen Shot 2023-03-17 at 12 00 10 AM

This won't be edited in the editor anyway.

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?

@ryelle
Copy link
Contributor Author

ryelle commented Mar 16, 2023

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).

@renintw
Copy link
Contributor

renintw commented Mar 16, 2023

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 🙂

@ryelle ryelle merged commit 87960be into trunk Mar 17, 2023
@ryelle ryelle deleted the update/header-footer-custom-colors branch March 17, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable color settings on the Footer block Border colors and behaviour in WordPress.org
3 participants