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

Quadrat: revise post header and meta #3824

Merged
merged 11 commits into from
May 11, 2021
Merged

Quadrat: revise post header and meta #3824

merged 11 commits into from
May 11, 2021

Conversation

jffng
Copy link
Contributor

@jffng jffng commented May 11, 2021

This PR addresses #3784.

Before After
Screen Shot 2021-05-10 at 8 32 56 PM Screen Shot 2021-05-10 at 8 31 59 PM

One thing I wasn't able to address is changing the divider between categories. This is baked into the block at the moment.

@jffng jffng requested review from a team and kjellr May 11, 2021 00:34
@jffng jffng linked an issue May 11, 2021 that may be closed by this pull request
@MaggieCabrera MaggieCabrera added this to the Quadrat v1 milestone May 11, 2021
<!-- wp:post-date {"textAlign":"center","fontSize":"tiny"} /-->
<!-- wp:post-tags {"textAlign":"center","fontSize":"tiny"} /-->
<!-- wp:spacer -->
<div style="height:170px" aria-hidden="true" class="wp-block-spacer"></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 it better to use padding on the group block? even margin now that we can do that? I'm writing this but I think I prefer spacers when the distance is so prominent so feel free to ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, for some reason using spacers feels a bit nicer when the spacing is large and opinionated like this. It makes the design more explicit via blocks.

@@ -199,9 +199,9 @@
},
"core/post-title": {
"typography": {
"fontSize": "min(max(36px, 5vw), 65px)",
"fontSize": "min(max(48px, 7vw), 80px)",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the same we are using for h1, should we make a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, perhaps. I will look at adding one.

Copy link
Contributor

@MaggieCabrera MaggieCabrera left a comment

Choose a reason for hiding this comment

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

This looks good to me in terms of code

@kjellr
Copy link
Contributor

kjellr commented May 11, 2021

One thing I wasn't able to address is changing the divider between categories. This is baked into the block at the moment.

That's weird! Can we adjust using CSS in the meantime?

@jffng
Copy link
Contributor Author

jffng commented May 11, 2021

@kjellr

Can we adjust using CSS in the meantime?

It looks like the divider is output in raw HTML (no wrapping or pseudo element), so I can't target / change its contents with CSS:

Screen Shot 2021-05-11 at 10 01 13 AM

I will look at submitting an improvement in Gutenberg (EDIT: opened an issue here WordPress/gutenberg#31710). In the meantime, is the rest of this PR okay to you?

I also just realized the way I'm adding the dot in between the date isn't good because it will always display, even if the post has no categories. So I will change that.


@MaggieCabrera MaggieCabrera linked an issue May 11, 2021 that may be closed by this pull request
@kjellr
Copy link
Contributor

kjellr commented May 11, 2021

While that issue gets sorted out, can you do something like this pseudocode to hide the pipes?

.wp-block-post-terms {
 color: var(--background);
}

.wp-block-post-terms a {
 color: var(--foreground);
}

@jffng
Copy link
Contributor Author

jffng commented May 11, 2021

Okay, here's what the post header is like now:

Desktop Mobile
Screen Shot 2021-05-11 at 12 08 13 PM Screen Shot 2021-05-11 at 12 09 33 PM

(Header + mobile menu styling will be addressed separately)

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Added some small adjustments to even things out. Once those are in place, I think this is good to merge. We may want to make some adjustments on mobile somehow, since some of the spacing seems extra-large there. But we can reassess in a separate PR.

Screen Shot 2021-05-11 at 1 25 13 PM

quadrat/block-template-parts/single.html Outdated Show resolved Hide resolved
quadrat/block-template-parts/single.html Outdated Show resolved Hide resolved
quadrat/block-template-parts/single.html Outdated Show resolved Hide resolved
jffng and others added 3 commits May 11, 2021 11:43
Co-authored-by: Kjell Reigstad <kjell.reigstad@automattic.com>
Co-authored-by: Kjell Reigstad <kjell.reigstad@automattic.com>
Co-authored-by: Kjell Reigstad <kjell.reigstad@automattic.com>
@jffng jffng merged commit 0c26b06 into trunk May 11, 2021
@jffng jffng deleted the fix/post-header branch May 11, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants