Skatepark: add the header #4354

jffng · 2021-08-02T21:06:45Z

Changes proposed in this Pull Request:

This PR adds a header template part and styles per the design for Skatepark. It applies some ideas from the every-layout.dev book. It was pretty interesting to explore and grok the concept of truly flexible layouts not tied to breakpoints.

Looking at Skatepark's header design and the components involved, there are three states to consider:

A. Not wrapped	B. Wrapped	C. Wrapped w/ responsive navigation

B is the interesting scenario. Before, I would have tried to write a media query that tells the header to break onto two lines at a specific pixel value. But this is not a great approach because we don't know for certain the content width / number of menu items, thus making it impossible to write a media query that solves for all the situations.

I could not get around using a media query for the navigation + social links, which I think is acceptable because the navigation block's responsive behavior is tied to the same media query.

The spacing needs some tightening to match the comps, but I wanted to get your feedback in the meantime.

Related issue(s):

#4313

jffng · 2021-08-03T16:53:43Z

skatepark/block-template-parts/header.html

+
+<!-- wp:group -->
+<div class="wp-block-group nav-links"><!-- wp:navigation {"orientation":"horizontal","itemsJustification":"right","isResponsive":true,"style":{"typography":{"fontStyle":"normal","fontWeight":"900","textTransform":"uppercase"}},"fontSize":"small"} -->
+<!-- wp:navigation-link {"label":"Home","type":"custom","url":"http://skateparkdev.local/","kind":"custom","isTopLevelLink":true} /-->


Not suggesting we ship the theme with these links baked in, but I included them so it's maybe easier to test.

skatepark/sass/templates/_header.scss

MaggieCabrera · 2021-08-04T08:20:11Z

skatepark/sass/templates/_header.scss

+			align-items: center;
+
+			&::after {
+				content: "Menu";


We can't translate this :(

Could we use the lang attribute on the html tag to translate this? Something like:

html[lang="en-US"] { .nav-links { .wp-block-navigation__responsive-container-open { &::after { content: "Menu"; } } } }

I guess this would need to be added via a PHP template in an inline style.. Or is this horrible and hacky?

we could add this line of CSS as inline inside functions.php and wrap the string around the PHP translation function directly, no need to target the lang attribute. It's not much better, but it's another option

Ah nice ok! Well, we have options 😄

Ideally there is an option in the nav block itself to provide this label next to the responsive hamburger. I wonder if it's planned

MaggieCabrera · 2021-08-04T08:21:28Z

I love how this looks! And it's very simple, great job!

This is what I see:

The social icons could have the same treatment as the footer block so that they align with the menu. In this particular case, I'd add a class to the block, since the users may want to re build the header or use another theme's and we don't want the css to break that.

This is what the footer is doing:

	.wp-block-social-links.is-style-logos-only {
		margin-left: calc( -1 * ( 8px + 0.25em ) );
	}

If either the navigation or the social links are not present, this happens. The scenario with no social links is worth considering.

And right now Skatepark doesn't support a site logo, should we add support for it, @melchoyce If so, should we copy Quadrat's behavior for it?

mikachan

This looks ace! Agree with @MaggieCabrera's comments. Looking good in all browsers and resolutions. I agree with needing to use the media query for the navigation and social links, I can't think of another way we could do this.

mikachan · 2021-08-04T10:27:24Z

skatepark/sass/templates/_header.scss

+		> * { // Apply a stack layout (page 69 of the every-layout.dev PDF) 
+			margin-top: 20px;
+			margin-bottom: 20px;
+		}


This is cool, I love the idea of styling the context rather than the element itself.

mikachan · 2021-08-04T10:39:09Z

skatepark/sass/templates/_header.scss

+			align-items: center;
+
+			&::after {
+				content: "Menu";


Could we use the lang attribute on the html tag to translate this? Something like:

html[lang="en-US"] { .nav-links { .wp-block-navigation__responsive-container-open { &::after { content: "Menu"; } } } }

I guess this would need to be added via a PHP template in an inline style.. Or is this horrible and hacky?

melchoyce · 2021-08-04T16:19:55Z

Whoo 🎉

Minor notes:

The last icon in the social block has a right margin. Can we remove that margin for the last item in the menu?
The tagline's font-weight should be medium
Navigation items are missing some letter spacing (0.1em)

And right now Skatepark doesn't support a site logo, should we add support for it, @melchoyce If so, should we copy Quadrat's behavior for it?

Yes and yes, though I think I'll want to test once we have Quadrat's behavior in Skatepark.

jffng · 2021-08-04T19:31:18Z

skatepark/functions.php

@@ -40,5 +40,13 @@ function skatepark_support() {
 function skatepark_scripts() {
 	// Enqueue front-end styles.
 	wp_enqueue_style( 'skatepark-styles', get_stylesheet_directory_uri() . '/assets/theme.css', array( 'blockbase-ponyfill' ), wp_get_theme()->get( 'Version' ) );
+
+	// Allow the responsive menu label to be translated.
+	$menu_label_css = '


This feels very messy, which makes me think it should go in the block.

In the meantime, maybe it should live in blockbase.

but blockbase doesn't have the Menu text. I think this is probably planned for the navigation block

Ok, I'm going to remove it from the theme for now and open an issue for it in Gutenberg.

jffng · 2021-08-04T19:32:04Z

skatepark/theme.json

@@ -381,7 +381,8 @@
 			},
 			"core/navigation": {
 				"typography": {
-					"fontSize": "var(--wp--preset--font-size--normal)"
+					"fontSize": "var(--wp--preset--font-size--normal)",
+					"letterSpacing": "0.05em"


@melchoyce I added the letter spacing for all navigation blocks — is that okay, or do you think it should be targeted to the navigation block in the header only?

Also 0.1em for letter spacing looked like a lot compared to the comps, so I used 0.05 but let me know if you think it should be 0.1em.

jffng · 2021-08-04T19:34:17Z

Thanks for the reviews and comments!

I made the following changes:

Removed right margin from social links
Changed the flex alignment to handle the case when either no menu or social links are present
Made it so the menu label can be translated
Added a placeholder for the site logo in the template.

I think this is ready for another review, be sure to test out adding a logo.

skatepark/block-template-parts/header.html

MaggieCabrera · 2021-08-05T16:25:38Z

The logo has a few differences comparing to Quadrat:

melchoyce · 2021-08-05T17:14:57Z

Can we swap out the menu icon for a slightly thicker, three-bar version?

And if possible, open the menu underneath instead of overlaying the header:

If that's not possible, we can keep as-is — just feels especially annoying to open the menu on the left side and then close it on the right. Also happy with us playing with this later in a new PR.

jffng · 2021-08-05T18:27:20Z

The logo has a few differences comparing to Quadrat:

@melchoyce any guidance on the position of the logo across viewports? The header on Quadrat is different so I wasn't sure how to handle.

And if possible, open the menu underneath instead of overlaying the header:

I think this will be tricky / I'm not sure if it's possible with the nav block currently. We can at least move the position of the close button within the modal so it's the same location as the open button.

melchoyce · 2021-08-05T20:02:03Z

Would it work to display side-by-side on desktop, and stack on mobile?

MaggieCabrera · 2021-08-09T07:45:51Z

Just adding that this also closes #4319 since I don't think we need a separate issue for that

jffng · 2021-08-10T18:20:45Z

Would it work to display side-by-side on desktop, and stack on mobile?

I gave this a try. Unless there is a purely flex based approach that I am missing, I don't think it's worth it because it would require another group block and more media-query based CSS, which is very fragile when we open things to the site editor.

The tagline's font-weight should be medium

I opened a small change in Gutenberg to allow this so we don't have to add CSS for it and the user can customize it: WordPress/gutenberg#33983

I also removed the responsive menu label in favor of adding the ability to the nav block itself: WordPress/gutenberg#33985

Thoughts on bringing this in and iterating?

mikachan · 2021-08-11T10:32:18Z

This is looking good for me in all browsers and resolutions. I've tested with and without both a logo and a menu.

Bringing this in and iterating sounds good to me.

MaggieCabrera

+1 on bringing this in and iterating. And I also agree that the menu label is best handled on the editor.

jffng added the [Theme] Skatepark Automatically generated label for Skatepark. label Aug 2, 2021

jffng linked an issue Aug 3, 2021 that may be closed by this pull request

Skatepark: Templates:Header template part (including mobile) #4313

Closed

jffng marked this pull request as ready for review August 3, 2021 16:51

jffng requested review from a team and melchoyce and removed request for melchoyce August 3, 2021 16:51

jffng changed the title ~~Create the scaffolding for header template part.~~ Skatepark: add the header Aug 3, 2021

jffng commented Aug 3, 2021

View reviewed changes

MaggieCabrera reviewed Aug 4, 2021

View reviewed changes

skatepark/sass/templates/_header.scss Show resolved Hide resolved

MaggieCabrera reviewed Aug 4, 2021

View reviewed changes

mikachan reviewed Aug 4, 2021

View reviewed changes

jffng force-pushed the add/skatepark-header branch from dfe8214 to f3b4de4 Compare August 4, 2021 18:14

jffng commented Aug 4, 2021

View reviewed changes

MaggieCabrera reviewed Aug 5, 2021

View reviewed changes

skatepark/block-template-parts/header.html Outdated Show resolved Hide resolved

MaggieCabrera linked an issue Aug 9, 2021 that may be closed by this pull request

Skatepark: Navigation (if needed) #4319

Closed

jffng added 6 commits August 10, 2021 12:33

Create the scaffolding for header template part.

62bd392

Use intrinsic design layouts for the primary header container.

c24f5a7

Simplify flex wrapping rules.

0c83ec1

Comments for everylayout.dev pages.

110e4c0

Use the Gutenberg flex layout.

54e9894

Refine header styles and translate the menu label.

c78bd36

jffng added 3 commits August 10, 2021 12:33

Add site logo.

be538f1

Use css var instead of baked color.

35010fd

Move flex layout to group so we can reuse templates from blockbase.

e8ec881

jffng force-pushed the add/skatepark-header branch from 9534e37 to e8ec881 Compare August 10, 2021 16:33

jffng added 3 commits August 10, 2021 13:17

Comment why flex grow is needed.

f5a511b

Use font-weight support in Gutenberg.

bf7a9b2

Remove menu label in favor of adding it in Gutenberg.

aaf1c80

Remove hard coded nav links.

b57cf3c

jffng requested a review from a team August 10, 2021 18:51

MaggieCabrera approved these changes Aug 11, 2021

View reviewed changes

jffng merged commit 1cb8b71 into trunk Aug 11, 2021

jffng deleted the add/skatepark-header branch August 11, 2021 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skatepark: add the header #4354

Skatepark: add the header #4354

jffng commented Aug 2, 2021 •

edited

Loading

jffng Aug 3, 2021

MaggieCabrera Aug 4, 2021

mikachan Aug 4, 2021

MaggieCabrera Aug 4, 2021

mikachan Aug 4, 2021

jffng Aug 4, 2021

MaggieCabrera commented Aug 4, 2021

mikachan left a comment

mikachan Aug 4, 2021

mikachan Aug 4, 2021

melchoyce commented Aug 4, 2021 •

edited

Loading

jffng Aug 4, 2021 •

edited

Loading

MaggieCabrera Aug 5, 2021

jffng Aug 10, 2021

jffng Aug 4, 2021

jffng Aug 5, 2021

jffng commented Aug 4, 2021 •

edited

Loading

MaggieCabrera commented Aug 5, 2021

melchoyce commented Aug 5, 2021

jffng commented Aug 5, 2021

melchoyce commented Aug 5, 2021

MaggieCabrera commented Aug 9, 2021

jffng commented Aug 10, 2021

mikachan commented Aug 11, 2021

MaggieCabrera left a comment

Skatepark: add the header #4354

Skatepark: add the header #4354

Conversation

jffng commented Aug 2, 2021 • edited Loading

Changes proposed in this Pull Request:

Related issue(s):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaggieCabrera commented Aug 4, 2021

mikachan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melchoyce commented Aug 4, 2021 • edited Loading

jffng Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jffng commented Aug 4, 2021 • edited Loading

MaggieCabrera commented Aug 5, 2021

melchoyce commented Aug 5, 2021

jffng commented Aug 5, 2021

melchoyce commented Aug 5, 2021

MaggieCabrera commented Aug 9, 2021

jffng commented Aug 10, 2021

mikachan commented Aug 11, 2021

MaggieCabrera left a comment

Choose a reason for hiding this comment

jffng commented Aug 2, 2021 •

edited

Loading

melchoyce commented Aug 4, 2021 •

edited

Loading

jffng Aug 4, 2021 •

edited

Loading

jffng commented Aug 4, 2021 •

edited

Loading