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

Bundle new collection of Header and Footer block patterns #43157

Merged
merged 16 commits into from
Aug 18, 2022

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Aug 11, 2022

What?

Authored by @beafialho and @kjellr, these block patterns represent a consolidation of recurring headers and footers, simplified in order to work well across themes.

As they go hand in hand with the new site-editing features rolling out this year, the goal is to bundle these in WordPress 6.1 soon, after testing in Gutenberg.

Props beafialho, kjellr

Testing Instructions

  • In the Site Editor, insert the new patterns using either inserter (global inserter, in-between inserter), and verify that they render properly in the canvas, in the inserter preview, and in the front end.
  • Alternatively, in the Site Editor canvas, select a Template Part block of type Header or Footer, and in its toolbar click "Replace header/toolbar". In the resulting modal screen, verify that the new patterns are correctly displayed, and that they match the type (i.e. only Footer patterns are offered to replace a Footer template part).
  • Please report any instances where the preview of a pattern could be improved (by adding a viewportWidth value).

Issues

  • Host the one image asset in an adequate place like s.w.org
  • Some patterns are using Navigation blocks with a ref, resulting in a broken pattern:
    • header-with-large-font-size
    • centered-logo-in-navigation
    • simple-header-with-background-color
  • Rename some incorrectly named files:
    • centered-headercentered-logo-in-navigation and vice versa
    • sample-pagesimple-header

@mcsf mcsf added the [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced label Aug 11, 2022
@mcsf mcsf requested review from mtias, ntsekouras and beafialho August 11, 2022 15:03
@mcsf mcsf requested a review from spacedmonkey as a code owner August 11, 2022 15:03
@beafialho
Copy link

Thanks for handling this so quickly! The new patterns look and work well in my testing. I have a couple comments:

  • One small adjustment on the "Simple header with background color", it had too much padding. Sorry I didn't see this before. I've updated the block code here.

  • The "Logo, navigation, and social icons" doesn't seem to have the social icons block there.

Even though I'm not sure this is the right place to address this, I have some thoughts on the browsing and categorisation of the patterns:

  • There are some default patterns in the "Headers" category that aren't really headers, they're hero sections. These should probably go in another category: "Pages" or "Featured" perhaps?

Captura de ecrã 2022-08-11, às 16 42 57

  • It could be nice to create a category of patterns called "Twenty Twenty Two" or move them to "Featured". TT2 has many patterns, very similar between them and also very similar with the new set. I guess their prominence, or perhaps the imagery makes me feel a bit distracted if I'm looking for certain header and footer elements. On the other hand, maybe it makes sense to have all of them in the same place. Just a thought.

@carolinan
Copy link
Contributor

When I tried to add author patterns, Kjellr told me the core patterns should not be in Gutenberg, but in the pattern directory. It would be good to establish what is the correct way.

'content' => '<!-- wp:group {"align":"full"} -->
<div class="wp-block-group alignfull"><!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"4em","bottom":"2em"}}},"layout":{"inherit":false}} -->
<div class="wp-block-group alignfull" style="padding-top:4em;padding-bottom:2em"><!-- wp:paragraph {"align":"center"} -->
<p class="has-text-align-center">Proudly Powered by <a href="https://wordpress.org">WordPress</a></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are using PHP in the block pattern, and the translation function is available, I think it would be good idea to make this text and the link translatable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, as well as in the other patterns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 0836d91. What do you think?

<div class="wp-block-group alignfull" style="padding-top:30px;padding-right:30px;padding-bottom:30px;padding-left:30px;"><!-- wp:site-title {"textAlign":"center","fontSize":"large"} /-->

<!-- wp:paragraph {"align":"center"} -->
<p class="has-text-align-center">Proudly Powered by <a href="https://wordpress.org">WordPress</a></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why the data-type="URL" and data-id are used inconsistently, on only some external links?
Does the pattern benefit from these?

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'll defer to @beafialho. If there are no specific reasons, we should just be consistent and boring. I am not familiar with this data-type / data-id practice.

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 took the liberty of removing those attributes along with other i18n changes in 0836d91

'content' => '<!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"30px","right":"30px","bottom":"30px","left":"30px"}}},"layout":{"type":"flex","flexWrap":"nowrap","justifyContent":"space-between"}} -->
<div class="wp-block-group alignfull" style="padding-top:30px;padding-right:30px;padding-bottom:30px;padding-left:30px;"><!-- wp:site-title {"style":{"typography":{"fontStyle":"normal","fontWeight":"700"}},"fontSize":"large"} /-->

<!-- wp:navigation {"ref":31,"layout":{"type":"flex","justifyContent":"space-between"},"style":{"spacing":{"blockGap":"30px"}},"fontSize":"large"} /--></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the practice is to remove the ref -There might not be a navigation menu with the ID "31" on the install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these aren't meant to be here (see "Issues" in the PR description).

<!-- wp:site-title {"style":{"elements":{"link":{"color":{"text":"var:preset|color|background"}}}}} /--></div>
<!-- /wp:group -->

<!-- wp:navigation {"ref":31,"__unstableLocation":"primary","layout":{"type":"flex","setCascadingProperties":true,"justifyContent":"right"}} /--></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only pattern that has an "__unstableLocation" for the navigation block? and should the patterns use it? I believe it is still experimental, considering it is still called "unstable" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as in the other comment: this Nav block should be rewritten without ref and site-dependent attributes.


<!-- wp:site-tagline {"textAlign":"center"} /-->

<!-- wp:spacer {"height":8} -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the spacer block needs a unit in the height attribute to be valid.
<!-- wp:spacer {"height":"8px"} -->
Was this removed on purpose? I suppose it depends what version of Gutenberg/Core that the pattern needs to support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye. I bet that was accidental.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed a few of these in 453c77f

'title' => _x( 'Footer with Large Font Size', 'Block pattern title' ),
'blockTypes' => array( 'core/template-part/footer' ),
'categories' => array( 'footer' ),
'content' => '<!-- wp:group {"align":"full","style":{"spacing":{"blockGap":"15px","padding":{"top":"30px","right":"30px","bottom":"30px","left":"30px"}}},"layout":{"type":"flex","orientation":"vertical","justifyContent":"left"}} -->
Copy link
Contributor

@carolinan carolinan Aug 15, 2022

Choose a reason for hiding this comment

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

Not a blocker of course, but this inconsistency of some patterns using em and this pattern using px for the spacing is a bit strange.

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'll defer to @beafialho again. :)

Choose a reason for hiding this comment

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

I usually work with px. Can we apply the equivalent em values?

@mcsf
Copy link
Contributor Author

mcsf commented Aug 15, 2022

When I tried to add author patterns, Kjellr told me the core patterns should not be in Gutenberg, but in the pattern directory. It would be good to establish what is the correct way.

Correct. Inclusion in the Pattern Directory was also the goal for this collection. However, the Directory's pattern editor is not yet able to handle several site blocks, as they affect site options and other privileged data. If we wait for that support in the directory, we will miss the window for WP 6.1. So, after a discussion with Bea, and due to the beneficial impact that these patterns could have for users in 6.1, @mtias recommended that we include these patterns even if it means bundling in Gutenberg+Core.

@mcsf
Copy link
Contributor Author

mcsf commented Aug 16, 2022

  • One small adjustment on the "Simple header with background color", it had too much padding. Sorry I didn't see this before. I've updated the block code here.

Done in 5f18442

  • The "Logo, navigation, and social icons" doesn't seem to have the social icons block there.

Hm, I don't see it. Or rather: the markup for each icon is missing from the pattern, but this is because the Social Link block is dynamic. On the other hand, the Social Links (plural) block is purely static. But you should be seeing the icons properly rendered in the editor and previews. Are you not?

Even though I'm not sure this is the right place to address this, I have some thoughts on the browsing and categorisation of the patterns:

  • There are some default patterns in the "Headers" category that aren't really headers, they're hero sections. These should probably go in another category: "Pages" or "Featured" perhaps?

Which ones do you see as not quite headers? (Also, feel free to make changes directly!)

  • It could be nice to create a category of patterns called "Twenty Twenty Two" or move them to "Featured". TT2 has many patterns, very similar between them and also very similar with the new set. I guess their prominence, or perhaps the imagery makes me feel a bit distracted if I'm looking for certain header and footer elements. On the other hand, maybe it makes sense to have all of them in the same place. Just a thought.

I'm not sure what you mean: are you suggesting adding a new category that gets registered by the TT2 theme? Or actually in Core? Is the issue that patterns from the bundle cannibalise TT2 patterns or vice-versa?

@beafialho
Copy link

On the other hand, the Social Links (plural) block is purely static. But you should be seeing the icons properly rendered in the editor and previews. Are you not?

No, I don't see the Social Links (plural) block at all. But if you see them, it's probably something with the site I'm using to test it.

Which ones do you see as not quite headers?

These:

184175138-a04ed824-e643-49cd-b2c0-b2cfd9677cde

(Also, feel free to make changes directly!)

I would make the changes myself but I don't have the technical knowledge to do so.

I'm not sure what you mean: are you suggesting adding a new category that gets registered by the TT2 theme? Or actually in Core? Is the issue that patterns from the bundle cannibalise TT2 patterns or vice-versa?

I'm thinking it would be nice to see the TT2 patterns together in its own category, not mixed with the other headers and footers. But I realize that can be confusing for some users so we can leave them as they are now. I don't feel strongly about it.

@mcsf
Copy link
Contributor Author

mcsf commented Aug 16, 2022

Which ones do you see as not quite headers?

These:

Ah, but those aren't part of the PR. :)

@mcsf
Copy link
Contributor Author

mcsf commented Aug 16, 2022

Some patterns are using Navigation blocks with a ref, resulting in a broken pattern

@beafialho: I haven't fixed these because I wanted your input. I could systematically replace them with something else, but usage is inconsistent across the remaining patterns:

  • Some, like centered-logo-in-navigation, create their own menu using Navigation Link blocks for About, Contact, etc.
  • Many others defer to a dynamic Page List, which in my case results in a menu with a single lonely "Page Sample" item.

The former seems more desirable to showcase the pattern, while the latter might be more ergonomic if users expect the editor to populate the menu with meaningful data. There is a lot of movement around the navigation blocks (e.g. how to best support theme switches, how to reconcile static and dynamic menus) that I haven't followed, and someone in the know might have better recommendations.

@mcsf
Copy link
Contributor Author

mcsf commented Aug 16, 2022

@ryelle: could you possibly help us get this image asset uploaded in https://s.w.org/patterns/files/ or similar? 🙏 🙏

https://headerandfooterpatterns.files.wordpress.com/2022/01/5eca6-header-mountains-scaled-1.jpg

@ryelle
Copy link
Contributor

ryelle commented Aug 16, 2022

Sure, here you go: https://s.w.org/patterns/files/2022/08/5eca6-header-mountains-scaled-1.jpg

@mcsf
Copy link
Contributor Author

mcsf commented Aug 16, 2022

Sure, here you go: https://s.w.org/patterns/files/2022/08/5eca6-header-mountains-scaled-1.jpg

Thanks! Pushed!

@mtias
Copy link
Member

mtias commented Aug 16, 2022

The former seems more desirable to showcase the pattern, while the latter might be more ergonomic if users expect the editor to populate the menu with meaningful data.

Patterns that aim to display whatever is the user's main menu should leave an empty navigation block (no id, no inner blocks); patterns that have a very specific layout should use Link Item at will with whatever placeholders make the most individual sense.

@carolinan
Copy link
Contributor

When I tried to add author patterns, Kjellr told me the core patterns should not be in Gutenberg, but in the pattern directory. It would be good to establish what is the correct way.

Correct. Inclusion in the Pattern Directory was also the goal for this collection. However, the Directory's pattern editor is not yet able to handle several site blocks, as they affect site options and other privileged data. If we wait for that support in the directory, we will miss the window for WP 6.1. So, after a discussion with Bea, and due to the beneficial impact that these patterns could have for users in 6.1, @mtias recommended that we include these patterns even if it means bundling in Gutenberg+Core.

And that will continue to be true for any new block, and the process needs to be established and documented.

@mcsf
Copy link
Contributor Author

mcsf commented Aug 17, 2022

Patterns that aim to display whatever is the user's main menu should leave an empty navigation block (no id, no inner blocks); patterns that have a very specific layout should use Link Item at will with whatever placeholders make the most individual sense.

What I ended up doing was rewriting all Navigation blocks as empty, while keeping any layout attributes, but discarding __unstableLocation. The exceptions to this were:

  • logo-navigation-and-social-icons.php, whose Navigation block combines Page List with Social Icons
  • centered-logo-in-navigation.php, whose Navigation block arranges a Site Logo in the middle of Navigation Link blocks.

FYI, the current UX for empty Navigation blocks could be better, but it's being iterated on by other contributors with WP 6.1 as the target. This also means that, for the time being, previews are less useful if the pattern contains an empty Navigation block.

I think this should be good to go from a technical standpoint. The patterns themselves should be good enough, too, but I'd like a fresh set of eyes to actually test them all. Is anything else needed here, or will we then be able to merge?

Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

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

Let's get it in, we'd want to test and refine until we can narrow down a set that makes sense to offer for headers and footers out of the box

@mcsf
Copy link
Contributor Author

mcsf commented Aug 17, 2022

Let's get it in, we'd want to test and refine until we can narrow down a set that makes sense to offer for headers and footers out of the box

Wonderful. FYI, per our private conversation I removed logo-navigation-and-social-icons in 0d17e058da2ba22a072a5a7c536ec554e9add748.

Waiting for the lights to go green, then 🚢

mcsf added 7 commits August 18, 2022 12:33
Exceptions:

- logo-navigation-and-social-icons.php, whose Navigation block combines
  Page List with Social Icons

- centered-logo-in-navigation.php, whose Navigation block arranges a
  Site Logo in the middle of Navigation Link blocks.
It can be added back in once it is rewritten in such a way that the
Social Icons block is not a child of Navigation, but its direct sibling,
while preserving the right-aligned design.
@mcsf mcsf force-pushed the add/patterns-header-and-footer-collection branch from 0d17e05 to a641f63 Compare August 18, 2022 11:33
@mcsf mcsf merged commit 3ee784b into trunk Aug 18, 2022
@mcsf mcsf deleted the add/patterns-header-and-footer-collection branch August 18, 2022 17:23
@mcsf
Copy link
Contributor Author

mcsf commented Aug 18, 2022

Thanks for all the feedback, everyone!

@github-actions github-actions bot added this to the Gutenberg 14.0 milestone Aug 18, 2022
mcsf added a commit that referenced this pull request Aug 19, 2022
mcsf added a commit that referenced this pull request Aug 19, 2022
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Sep 4, 2022
@Mamaduka
Copy link
Member

Mamaduka commented Nov 7, 2022

Based on conversations in #44843, I'm bumping this to WP 6.2.

I will follow up with PR to update the compat directory for these files so changes aren't missed during the 6.2 release cycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced Needs User Documentation Needs new user documentation
Projects
No open projects
Status: Bumped to 6.2
Development

Successfully merging this pull request may close these issues.

7 participants