-
Notifications
You must be signed in to change notification settings - Fork 92
Conversation
<!-- wp:site-title {"style":{"typography":{"fontStyle":"italic","fontWeight":"400"}}} /--></div> | ||
<!-- /wp:group --> | ||
|
||
<!-- wp:navigation {"itemsJustification":"right","isResponsive":true} --> |
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.
If I update the navigation in the standard header, and I go to insert a new header from one of these patterns, it pulls in the old / original navigation. Is there a way to get the pattern to pull in a navigation template part, i.e. the nav that I've updated already?
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.
Not yet! But there's some early work in WordPress/gutenberg#35418 to help make that sort of thing happen. I suggest we hold on and see how that shakes out before implementing any sort of workarounds here.
Fixed in b047708 via some block margin adjustments. 👍 |
After chatting on slack with @mtias and @jasmussen, I've pushed mostly-working versions of the four patterns that were marked with an ❌ to this PR as well. I've added them to the end of the patterns list. Let's merge them in for now and open a followup issue to address issues. For now, here they are along with the known bugs: Header with centered Logo Current issues:
Header with centered Logo in Navigation Current issues:
Centered title with Navigation and Social Links Header Current issues:
Title and Button Header Current issues:
This should be ready for a review. If they generally look good, I suggest we follow up with issues for everything above and work from there. |
The ones that stack awkwardly in mobile are good ways to test solutions for auto-collapsing containers. |
Quite a lot of the patterns use the same spacing values. What do you think about making these variables in the |
For the header and the header patterns, what do you think about using a |
I considered this, but in the interest of portability, I though we should put numerical values in there for now. If we do make these patterns available in the directory, we can't guarantee that a users' theme has that variable defined. In any case, if we decide to add that later, it's a quick find and replace.
I thought about this too. We can't guarantee where the user places these patterns, so I wasn't sure if it made sense to mark them as headers? We can probably expect that these are headers since they're labeled as such. I'll update so that it uses the |
'title' => __( 'Header with centered Logo', 'twentytwentytwo' ), | ||
'categories' => array( 'twentytwentytwo-headers' ), | ||
'blockTypes' => array( 'core/template-part/header' ), | ||
'content' => '<!-- wp:group {"align":"wide","tagName":"header","style":{"spacing":{"padding":{"bottom":"8rem","top":"max(1.25rem, 5vw)"}}}} --> |
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.
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.
Yeah, I'm unsure of the best way to handle left-right padding for these, but I think leaving it out makes the most sense for now. As I mentioned above:
I opted not to include any left/right padding around the patterns themselves. In our templates I've been adding that to the parent template part, since it needs to match up with the padding around the
main
template too. Keeping left/right padding out of the patterns seems more versatile than having it in there.
... which I still feel is true. Because for instance, the user will want the same padding around the entire page — the footer, page content, etc.
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.
Should the new site padding be used?
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.
Yes, it should. 😄 I'll work that into a separate PR.
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.
FWIW, I tried this, but it currently leaves us in a worse state than we were in before — the header with the background color isn't possible yet: #88
'title' => __( 'Centered title with Navigation and Social Links Header', 'twentytwentytwo' ), | ||
'categories' => array( 'twentytwentytwo-headers' ), | ||
'blockTypes' => array( 'core/template-part/header' ), | ||
'content' => '<!-- wp:group {"align":"wide","tagName":"header","style":{"spacing":{"padding":{"bottom":"8rem","top":"max(1.25rem, 5vw)"}}}} --> |
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.
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.
Oh that's weird. This only happens if you add a header
block first, and then choose these from the carousel. Then they appear wrapped in a header
. I guess the solution is to remove the header tagName
here.
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.
Should be all set as of 473937b.
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.
I think this is a good spot to bring in and we can iterate, since other PRs are relying on it too.
Note that some of the references below are not the only contributions (commits, PR review, helpful issue input, etc.) from these folks, but merely the first ones I encountered while reviewing items committed to `trunk` since the initial branch commit. @westonruter via WordPress#51 @ntwb via WordPress#73 @juricav via WordPress#113 @Sandstromer via WordPress#69 @jasmussen via WordPress#74 @melchoyce via WordPress#16 @Riyadh1734 via WordPress#182 @desrosj via WordPress#223 @beafialho via WordPress#172 @clucasrowlands via WordPress#171 @Otto42 via WordPress#28 @luminuu via WordPress#107 @felixarntz via WordPress#240
* add missing props to CONTRIBUTORS.md Note that some of the references below are not the only contributions (commits, PR review, helpful issue input, etc.) from these folks, but merely the first ones I encountered while reviewing items committed to `trunk` since the initial branch commit. @westonruter via #51 @ntwb via #73 @juricav via #113 @Sandstromer via #69 @jasmussen via #74 @melchoyce via #16 @Riyadh1734 via #182 @desrosj via #223 @beafialho via #172 @clucasrowlands via #171 @Otto42 via #28 @luminuu via #107 @felixarntz via #240 * add dotorg handles to CONTRIBUTORS.md * Fix Rich's WP.org username. Co-authored-by: Kjell Reigstad <kjell@kjellr.com>
Description
Addresses #14. (Probably closes it for now at least). As per #12, we may move these to the directory at some point, but I think it makes sense to start building them in here to get started.
This PR adds 12 of the block patterns from the initial designs:
The four that are marked with an ❌ were not possible to create with the current block controls. I think that's fine though... we can leave them out for now.
Some technical notes:
header
andheader-large-dark
patterns as used in our block template parts. Theme template parts don't currently show up in the theme carousel, so I think this is a sound strategy? That should probably change though.The vertical gap between the title and tagline on Header with Tagline and Text-only Header with stacked Tagline is clearly too big. I'm not sure if we can adjust that yet though?main
template too. Keeping left/right padding out of the patterns seems more versatile than having it in there.header.html
andheader-large-black.html
template parts is supposed to be italic in order to match the comps. I made that change here to sync things up.Screenshots
Testing Instructions