Skip to content
This repository was archived by the owner on Feb 17, 2025. It is now read-only.

Blockbase: fixed spacing for header wide#5472

Merged
MaggieCabrera merged 1 commit into
trunkfrom
blockbase-fix-header-wide
Feb 11, 2022
Merged

Blockbase: fixed spacing for header wide#5472
MaggieCabrera merged 1 commit into
trunkfrom
blockbase-fix-header-wide

Conversation

@MaggieCabrera

Copy link
Copy Markdown
Contributor

Changes proposed in this Pull Request:

Header wide is used exclusively by Skatepark by default. The spacing between the items was too tight, this was caused by the gap value on Skatepark being too small compared to other themes.

I removed an extra CSS rule on Blockbase that adds gap to the navigation, this is no longer necessary after the latest changes to the block. I tested on other themes and neither where affected by this change.

Before:

Screenshot 2022-02-08 at 10 38 31

After:
Screenshot 2022-02-08 at 10 32 55

Related issue(s):

#5471

@MaggieCabrera MaggieCabrera requested review from a team and kjellr February 8, 2022 09:39
@MaggieCabrera MaggieCabrera self-assigned this Feb 8, 2022
@MaggieCabrera MaggieCabrera added [Theme] Skatepark Automatically generated label for Skatepark. [Theme] Blockbase labels Feb 8, 2022

@scruffian scruffian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Should the top of the navigation also align with the top of the site title?

@MaggieCabrera

Copy link
Copy Markdown
Contributor Author

LGTM. Should the top of the navigation also align with the top of the site title?

I'm not sure if that would affect other stuff (the logo, the mobile behavior...) without adding extra CSS. In any case, @kjellr what do you think about the header as it stands on this PR?

@scruffian

Copy link
Copy Markdown
Member

I think we can merge this either way...

@kjellr

kjellr commented Feb 8, 2022

Copy link
Copy Markdown
Contributor

Should the top of the navigation also align with the top of the site title?

I would like it to, yes. If it's going to take CSS though, I'd rather we just removed the tagline to achieve that alignment.

Aside from that, is it possible to extend custom blockGap to the social links too? They don't need the same giant spacing as the rest of the nav, but just a little bit more would be good. They look a little cramped currently.

@MaggieCabrera

Copy link
Copy Markdown
Contributor Author

For the social links CSS would be needed since we are using Blockbase, I'm going to merge all Skatepark PRs and then make a new one on top of them that removes Blockbase as a parent, then we can cover the header issues again.

@MaggieCabrera MaggieCabrera merged commit 71c3f66 into trunk Feb 11, 2022
@scruffian

Copy link
Copy Markdown
Member

Looking at this some more, I noticed that the block itself actually has a blockGap setting. I think we should try to get this fixed, so I am suggesting WordPress/wordpress-develop#2307

@scruffian scruffian deleted the blockbase-fix-header-wide branch February 11, 2022 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

[Theme] Blockbase [Theme] Skatepark Automatically generated label for Skatepark.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants