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

Add dark black header and footer variation #307

Closed

Conversation

bengreeley
Copy link

Currently, the header and footer blocks don't support the charcoal-1 color. This variation is needed for the Enterprise landing page.

To test:

  • Open a page in the editor
  • Add or edit the 'Global Header' and 'Global Footer' blocks
  • Set the variation to 'White on dark black'
  • Verify the header/footer blocks change color in the editor.
  • Preview and verify the blocks change color on the front end.

Screenshot 2022-12-08 at 12 44 33 PM

Screenshot 2022-12-08 at 12 39 38 PM

Ben Greeley added 3 commits December 8, 2022 12:32
@ryelle
Copy link
Contributor

ryelle commented Dec 9, 2022

This is probably more a question for Design (@jasmussen), but if we're introducing a new header style, do the hover colors need to be updated too? The other styles have a noticeably different hover/submenu background color, but the default black blends in with charcoal-1 (#1e1e1e vs #1c2024).

Default Blue White This PR
Screenshot 2022-12-09 at 10 19 14 AM Screenshot 2022-12-09 at 10 18 31 AM Screenshot 2022-12-09 at 10 18 50 AM Screenshot 2022-12-09 at 10 16 40 AM

background: var(--wp-global-header--background-color);
background: inherit;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than changing this, you can add another "case" to _common.pcss to override --wp-global-header--background-color. See how it's done for the other styles here:

--wp-global-header--background-color: var(--wp--preset--color--charcoal-2);
--wp-global-header--background-color--hover: var(--wp--preset--color--charcoal-5);
--wp-global-header--link-color: var(--wp--preset--color--white);
--wp-global-header--link-color--active: var(--wp--preset--color--blueberry-2);
--wp-global-header--text-color: var(--wp--preset--color--white);
}
.has-charcoal-2-color {
--wp-global-header--link-color: var(--wp--preset--color--charcoal-2);
--wp-global-header--link-color--active: var(--wp--preset--color--deep-blueberry);
--wp-global-header--text-color: var(--wp--preset--color--charcoal-2);
}
.has-blueberry-1-background-color {
--wp-global-header--background-color: var(--wp--preset--color--blueberry-1);
--wp-global-header--background-color--hover: var(--wp--preset--color--deep-blueberry);
--wp-global-header--link-color--active: var(--wp--preset--color--white);
}
.has-white-background-color {
--wp-global-header--background-color: var(--wp--preset--color--white);
--wp-global-header--background-color--hover: var(--wp--preset--color--light-grey-2);
}

Ben Greeley added 2 commits December 9, 2022 11:52
…or each variation were not changed to account for backwards-compatibility support.
@@ -13,9 +13,10 @@ import { useBlockProps } from '@wordpress/block-editor';
import metadata from './block.json';

const variations = [
{ label: __( 'White on black (default)', 'wporg' ), value: 'white-on-black' },
{ label: __( 'White on charcoal-1', 'wporg' ), value: 'white-on-dark-black' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is using this value yet, correct? So it's not too late to try to improve the naming? For me the range of values is where it's going to get messy if/when we add more variations...

Suggested change
{ label: __( 'White on charcoal-1', 'wporg' ), value: 'white-on-dark-black' },
{ label: __( 'White on charcoal-1', 'wporg' ), value: 'white-on-charcoal-1' },

Also I wonder if it's still manageable to migrate the existing usages to new values while we are still relatively early in the overall redesign. All the pages are saved as patterns in source control in the various repos, so we could replace eg. white-on-black with white-on-charcoal-2, and deploy all at once.

Or maybe I'm worrying too much about this... feel free to give me a reality check 😂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For white-on-black
1 usage in wporg-main-2022
2 usages in wporg-showcase-2022
2 usages in wporg-parent-2021
Several in this repo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usages are even better for white-on-blue

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamwoodnz Perhaps for a migration strategy we could do the following:

  1. Add both the legacy and new values to the switch statement so we can support both the legacy and new new names.
  2. Create an issue so we can update the content someday.
  3. Eventually remove the support from the switch statements.

What do you think? I'm going to hold off until we find out whether we are going to use charcoal-1 or charcoal-2, but I think it's a good idea to implement someday.

Copy link
Contributor

@adamwoodnz adamwoodnz Dec 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was a bit hesitant to suggest we reuse the same color. I think that makes sense for simplicity's sake.

Having both values in the switch could work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a deprecation work? It might only apply to JS blocks.

If so, I think the proposed migration strategy sounds good.

@jasmussen
Copy link
Collaborator

but if we're introducing a new header style, do the hover colors need to be updated too? The other styles have a noticeably different hover/submenu background color, but the default black blends in with charcoal-1 (#1e1e1e vs #1c2024).

Good question. I guess it depends on whether we can use charcoal 1 or 2 right? (related, #308)

I'd be happy to reuse the existing charcoal for simplicity, unless @panchovm has an opportunity to chime in otherwise.

@fcoveram
Copy link
Collaborator

Agree with @jasmussen on using the existing header scheme.

@bengreeley
Copy link
Author

Sounds good. In that case, I'm going to close this PR. If we want to implement the new naming convention, we always can in the future.

@bengreeley bengreeley closed this Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants