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

Blank Canvas Blocks: Added Block Patterns #3514

Merged
merged 32 commits into from
Mar 25, 2021

Conversation

MaggieCabrera
Copy link
Contributor

Changes proposed in this Pull Request:

This PR adds block patterns to BCB. I have only tested the patterns that load on .org and I had to make adjustments due to the change in color naming conventions.

Related issue(s):

Closes #3415

@scruffian
Copy link
Member

There are a couple of patterns that don't work for me:

Screenshot 2021-03-23 at 16 33 48

I do often have problems with cover blocks though, so this might just be me!

@MaggieCabrera
Copy link
Contributor Author

There are a couple of patterns that don't work for me:

Screenshot 2021-03-23 at 16 33 48

I do often have problems with cover blocks though, so this might just be me!

Nope! that happens when the block pattern is calling a color (background, primary) that doesn't exist on the current theme and it's asking you to choose a new one. I think you are using a plugin that I'm not cause I don't see those, I'll look for that.

<!-- /wp:image -->

<!-- wp:heading {"textAlign":"center","level":1,"textColor":"almost-black"} -->
<h1 class="has-text-align-center has-almost-black-color has-text-color"><strong>'. esc_html__( 'Jasmine Baker', 'blank-canvas-blocks' ) . '</strong></h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is no guarantee that 'almost-black' is going to be available in any child themes (or any theme once switched) I don't believe has-almost-black-color (or any of the descriptive color values) should be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could use just the hex color but this is not a universal block pattern, is it? according to our slack conversation, child themes are going to unregister these.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how all block patterns aren't "universal". If on theme A I use a block pattern, then change themes the blocks are still there but they they (potentially) look bad because classes that were expected are no longer there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's the problem if we don't use hex codes or until we get semantic colors that are available to all themes...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think for now we should use hex colors in all of the Blank Canvas patterns to combat this.

@scruffian
Copy link
Member

I think you are using a plugin that I'm not cause I don't see those, I'll look for that.

The only plugin I have is Gutenberg

'title' => __( 'Gradient Links', 'blank-canvas-blocks' ),
'categories' => array( 'link-in-bio' ),
'content' => '<!-- wp:cover {"overlayColor":"background","minHeight":1090,"minHeightUnit":"px","align":"full"} -->
<div class="wp-block-cover alignfull has-background-background-color has-background-dim" style="min-height:1090px"><div class="wp-block-cover__inner-container"><!-- wp:image {"align":"center","id":130,"width":96,"height":96,"sizeSlug":"large","linkDestination":"none","className":"is-style-rounded"} -->
Copy link
Contributor

Choose a reason for hiding this comment

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

has-background-background-color would be super if we had semantic colors define-able in our themes. But alas the "background color" is only available as a CSS variable since it's expressed in the "custom" block of theme.json. This color should probably be expressed another way.

'title' => __( 'Musician Links', 'blank-canvas-blocks' ),
'categories' => array( 'link-in-bio' ),
'content' => '<!-- wp:cover {"url":"' . esc_url( get_stylesheet_directory_uri() . '/assets/images/pattern-links-gradient.jpg' ) . '","id":181,"hasParallax":true,"dimRatio":0,"overlayColor":"primary","minHeight":100,"minHeightUnit":"vh","align":"full"} -->
<div class="wp-block-cover alignfull has-primary-background-color has-parallax" style="background-image:url(' . esc_url( get_stylesheet_directory_uri() . '/assets/images/pattern-links-gradient.jpg' ) . ');min-height:100vh"><div class="wp-block-cover__inner-container"><!-- wp:spacer {"height":10} -->
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no has-primary-background-color available to be used here.

@scruffian
Copy link
Member

I think this is ready for another test

pbking and others added 2 commits March 24, 2021 08:38
* Limit content width for things aligned left/right

Co-authored-by: Jeff Ong <jeff.ong@automattic.com>
@pbking
Copy link
Contributor

pbking commented Mar 24, 2021

Pushed a minor change to alignment as it related to containers that aren't groups that this branch illuminated.

image

@kjellr
Copy link
Contributor

kjellr commented Mar 24, 2021

Whoops — I posted some original notes over in #3415 (comment) before I realized this PR was here. This definitely improves upon that but I still have a bunch of notes:

  • There are content-width errors throughout.
  • The page title is present. (Should I be switching to a different template here? Not sure how that is supposed to work)
  • The spacing between blocks is very different than in the original theme. Can we adjust that?
  • The heading font weights don't match.
  • The separator blocks don't match the original theme.
  • The "Antonio Miller" heading is not readable.
  • The "Patricia Jones" logo image is missing for me.
  • The "Patricia Jones" link underlines are different, but I think that's ok.
Blank Canvas Blank Canvas Blocks
dotcomthemes test__p=1040 dotcomthemes test__p=1043

@jffng
Copy link
Contributor

jffng commented Mar 24, 2021

We made a number of changes to bring these patterns more in line with their designs:

  • Adjusted button hover + text colors
  • Adjusted font weights for headings — removing the "strong" tag / bold treatment from the pattern and using the custom font weight we are declaring for all headings.
  • Adjusted spacing variable to use the same value as Seedlet (30px) and applied it to paragraphs and heading elements.
  • Fixed width errors occurring in button groups by wrapping them inside a group block and inheriting the default layout.
  • Applied has-black-color has-text-color classes to Antonio Miller heading
  • Fixed Patricia Jones logo image
  • Adjusted separators

The page title is present. (Should I be switching to a different template here? Not sure how that is supposed to work)

My assumption is FSE handles this.

@@ -16,6 +16,10 @@ img {
max-width: 100%;
}

hr {
border: none; // Undo the wp-admin common.css: https://github.com/WordPress/WordPress/blob/fc6e4cbe3ef1dcd3a9977272d7ddcc413acdde0d/wp-admin/css/common.css#L865-L869
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR should undo what wp admin defines, but the browser defaults are not border: none; so it will be different if/when that PR lands. add_theme_support( 'wp-block-styles' ); does provide some defaults for the separator, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That PR just landed, by the way

@kjellr
Copy link
Contributor

kjellr commented Mar 25, 2021

My assumption is FSE handles this.

In general, if this is a 1:1 reproduction of Blank Canvas, we'll want the default post/page template to have no page title. We should definitely provide an alternate that does though. I see that in general, we need to add more templates, so this can totally be handled separately though.

I'll put this on my list to re-test today. 👍

@kjellr
Copy link
Contributor

kjellr commented Mar 25, 2021

These changes look good, and match the existing patterns pretty well. 🙌 Only a few comments:

@pbking
Copy link
Contributor

pbking commented Mar 25, 2021

Because we aren't able to customize what strong means without configuration we don't expect from Gutenberg I don't think we should do that in BCB. So for now that locks us in at (I think?) a font-weight of ~700 for <strong>. Which is a tad heavier than the 600 that Blank Canvas achieves but will be our only choice without theme-specific CSS overrides. (A child-them of this could.)

I suggest we revert the emboldened changes, keeping them a tad bolder than desired but retaining the ability to embolden headers with rich text controls.

Weights look like this now
image

@kjellr
Copy link
Contributor

kjellr commented Mar 25, 2021

Do we need a Gutenberg issue for allowing global styles to specify a font weight for bold? Or is that tracked already somewhere?

@pbking
Copy link
Contributor

pbking commented Mar 25, 2021

I don't believe so; I'm of the opinion that doing that in the font's CSS is appropriate. BCB is in a unique place where we're trying to avoid specifying things that aren't going to be controlled by Gutenberg (so subsequent child themes won't have to work to overcome it).

As far as I know an issue doesn't exist for that. If that seems like a thing that should be user or theme configurable - outside of CSS - I wouldn't argue against it; just seems abstract enough a thing to expect to be in a theme's limited specific CSS.

@pbking
Copy link
Contributor

pbking commented Mar 25, 2021

Regarding the button hover state, since the colors are applied with style properties there's no CSS solution. I don't believe the fix you reference is quite the same situation. I'm afraid that using that solution to style the buttons results in hover-less states.

I would certainly be keen to address that problem in some way but I'm not sure how. It should probably have something to do with the button block directly though.

@pbking pbking merged commit 46b64a3 into make/blank-canvas-blocks Mar 25, 2021
@pbking pbking deleted the add/bcb-block-patterns branch March 25, 2021 20:29
@kjellr
Copy link
Contributor

kjellr commented Mar 26, 2021

Regarding the button hover state, since the colors are applied with style properties there's no CSS solution. I don't believe the fix you reference is quite the same situation. I'm afraid that using that solution to style the buttons results in hover-less states.
I would certainly be keen to address that problem in some way but I'm not sure how. It should probably have something to do with the button block directly though.

We definitely fixed this for the normal version of Blank Canvas blocks — maybe it was in a followup PR actually? Or it could've been somewhere else. 🤔

In any case, we should make sure it's working the same way for Blank Canvas blocks, since I don't expect the hover states to be sorted out in Gutenberg for quite some time. The hover states were a blocking issue for launching the patterns back when we initially launched them, and I consider it a blocking issue for BCB too. I've opened #3538 to track it.

@kjellr
Copy link
Contributor

kjellr commented Mar 26, 2021

I've also opened a Gutenberg issue for the bold font weight setting: WordPress/gutenberg#30297

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.

6 participants