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

Blockbase: Update the stacking of site title and tagline in Blockbase and co #4928

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

scruffian
Copy link
Member

Changes proposed in this Pull Request:

This updates the way that site title and site tagline blocks stack in the header. I realised we could do this easily without any CSS. I think this is preferable as it will give a lot more space to the navigation before it stacks.

Blockbase:
Screenshot 2021-10-28 at 08 43 48

Quadrat:
Screenshot 2021-10-28 at 08 44 43

Geologist:
Screenshot 2021-10-28 at 08 52 35

Zoologist:
Screenshot 2021-10-28 at 09 06 30

Mayland blocks:
Screenshot 2021-10-28 at 08 53 45

Russell:
Screenshot 2021-10-28 at 08 55 11

Kerr:
Screenshot 2021-10-28 at 08 52 54

Related issue(s):

#4881

@scruffian scruffian requested review from kjellr, alaczek and a team October 28, 2021 08:07
@scruffian scruffian self-assigned this Oct 28, 2021
@MaggieCabrera
Copy link
Contributor

I agree with this if @beafialho and @kjellr give the thumbs up, it helps when either the site title, tagline or the menu are very long.

Skatepark doesn't seem to be aligned though, although this PR is not affecting it we should make it look like the rest:
Screenshot 2021-10-28 at 10 11 06

Looks like mobile works fine too with this change:

With tagline Without
Screenshot 2021-10-28 at 10 13 29 Screenshot 2021-10-28 at 10 13 46

@beafialho
Copy link
Collaborator

This looks good, except for Skatepark which is misaligning on mobile:

Captura de ecrã 2021-10-28, às 10 45 03

Here's how it should align, according to the Figma file:

Captura de ecrã 2021-10-28, às 10 48 13

@scruffian
Copy link
Member Author

I don't see that problem on Skatepark:
Screenshot 2021-10-28 at 13 25 25

Either way this PR doesn't touch skatepark, so I don't think that should stop us merging.

@pbking
Copy link
Contributor

pbking commented Oct 28, 2021

I tried to make it even more "blocky" by wrapping things in another row:

<!-- wp:group {"className":"site-header","layout":{"type":"flex","justifyContent":"space-between"},"style":{"spacing":{"padding":{"bottom":"80px"}}}} -->
<div class="wp-block-group site-header" style="padding-bottom:80px">

	<!-- wp:group {"layout":{"type":"flex"}} -->
	<div class="wp-block-group">
		<!-- wp:site-logo /-->

		<!-- wp:group -->
		<div class="wp-block-group">
			<!-- wp:site-title /-->
			<!-- wp:site-tagline {"style":{"typography":{"fontSize":"var(--wp--custom--font-sizes--tiny)"}}} /-->
		</div>
		<!-- /wp:group -->
	</div>
	<!-- /wp:group -->

	<!-- wp:navigation {"className":"is-style-blockbase-navigation-improved-responsive","itemsJustification":"right","isResponsive":true,"__unstableLocation":"primary","__unstableSocialLinks":"social"} /-->
</div>
<!-- /wp:group -->

Doing so allowed more of the header.scss to be cleaned out, however the design changing so that the logo was above and in-the-center wasn't do-able that way.

As it stands this change just updates the design, it doesn't simplify the header like I was hoping we could.

My 'druthers would be to drop the design (at least for Blockbase) where the mobile layout is different than desktop allowing the design to be more fully represented in block markup rather than CSS. But that's a bigger change then the design change done here and more impactful I think.

If the image needs to stay where it is this change looks good and we should ship it. All the themes I checked out looked correct.

@kjellr
Copy link
Contributor

kjellr commented Oct 28, 2021

I think we should keep the image where it is today on mobile. 👍

@scruffian scruffian merged commit 05b8d89 into trunk Oct 29, 2021
@scruffian scruffian deleted the update/header-stacking branch October 29, 2021 07:22
@kjellr
Copy link
Contributor

kjellr commented Oct 29, 2021

@scruffian, @pbking: Let's please revert this change. This has a very noticeable impact to users of these themes, and it also does not look right today anyway:

Screen Shot 2021-10-29 at 4 22 40 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants