Archeo: Image with headline and description pattern #5467

MaggieCabrera · 2022-02-07T09:50:57Z

Changes proposed in this Pull Request:

I made this using the media text block inside a full-width group block (for padding). @beafialho the image is exported with a white background, do you have one without it or with the background color of the site (in that case we may want the whole group block to have the same color as background for consistency)?

Related issue(s):

#5429

jffng

The code LGTM.

I have some minor comments / questions about the design:

The spacing between the heading and text looks tighter in the comp, should this be tightened?
The way it stacks on mobile is the image appears on top of the text, but the comp suggests it should be the other way around.

jffng · 2022-02-07T16:38:31Z

archeo/inc/patterns/image-with-headline-description.php

+return array(
+	'title'      => __( 'Image with headline and description', 'archeo' ),
+	'categories' => array( 'pages' ),
+	'content'    => '<!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"5vw","right":"5vw","bottom":"5vw","left":"5vw"}}}} -->


Should the bottom padding be 0, that way the image reaches all the way to the bottom of the group?

jffng · 2022-02-07T17:48:55Z

I made some changes:

Tightened the spacing between the heading and the text
Used a new responsive value for the heading that matches the "Gigantic" font size in the comps
Reduced the bottom padding of the group container.

The way it stacks on mobile is the image appears on top of the text, but the comp suggests it should be the other way around.

I couldn't figure out how to change the order of its stacking without adding CSS.

jffng · 2022-02-07T17:51:21Z

archeo/inc/patterns/image-with-headline-description.php

+	'categories' => array( 'pages' ),
+	'content'    => '<!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"5vw","right":"5vw","bottom":"0","left":"5vw"}}}} -->
+	<div class="wp-block-group alignfull" style="padding-top:5vw;padding-right:5vw;padding-bottom:0;padding-left:5vw"><!-- wp:media-text {"mediaPosition":"right","mediaLink":"' . esc_url( get_template_directory_uri() ) . '/assets/images/chahk.jpg","mediaType":"image","mediaWidth":64,"verticalAlignment":"top"} -->
+	<div class="wp-block-media-text alignwide has-media-on-the-right is-stacked-on-mobile is-vertically-aligned-top" style="grid-template-columns:auto 64%"><figure class="wp-block-media-text__media"><img src="' . esc_url( get_template_directory_uri() ) . '/assets/images/chahk.jpg" alt="' . esc_attr__( 'Chahk: rain deity', 'archeo' ) . '"/></figure><div class="wp-block-media-text__content"><!-- wp:heading {"style":{"typography":{"fontSize":"clamp(64px, 6vw, 100px)","lineHeight":"1"},"spacing":{"margin":{"bottom":"48px"}}}} --><h2 id="chahk-raindeity" style="font-size:clamp(64px, 6vw, 100px);line-height:1;margin-bottom:48px">' . wp_kses_post( __( 'CHAHK:<br>RAIN<br>DEITY', 'archeo' ) ) . '</h2>


I have a couple questions:

Should the font-size value be a custom variable in theme.json too?

Semantically, is it okay that this is an h2 instead of a paragraph? I changed it to a heading so that I could get margin control...

Should the font-size value be a custom variable in theme.json too?

I have the same question, and I think it would be great if it was a variable, I just don't know where it should sit in theme.json (Maybe just an extra font size? But then I don't think it's selectable from the editor.)

Semantically, is it okay that this is an h2 instead of a paragraph?

I think this is OK, as it's acting as a heading for this pattern.

mikachan · 2022-02-08T10:25:40Z

I couldn't figure out how to change the order of its stacking without adding CSS.

Hmm.. @kjellr adjusted the comps for us so this would work nicely, maybe we should change them back to the other way around? 😅

mikachan

This is looking great, thanks for picking it up!

The jpg is pretty big at 1.29MB, do we normally compress these before bundling them?

mikachan · 2022-02-08T10:30:00Z

archeo/inc/patterns/image-with-headline-description.php

+	'categories' => array( 'pages' ),
+	'content'    => '<!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"5vw","right":"5vw","bottom":"0","left":"5vw"}}}} -->
+	<div class="wp-block-group alignfull" style="padding-top:5vw;padding-right:5vw;padding-bottom:0;padding-left:5vw"><!-- wp:media-text {"mediaPosition":"right","mediaLink":"' . esc_url( get_template_directory_uri() ) . '/assets/images/chahk.jpg","mediaType":"image","mediaWidth":64,"verticalAlignment":"top"} -->
+	<div class="wp-block-media-text alignwide has-media-on-the-right is-stacked-on-mobile is-vertically-aligned-top" style="grid-template-columns:auto 64%"><figure class="wp-block-media-text__media"><img src="' . esc_url( get_template_directory_uri() ) . '/assets/images/chahk.jpg" alt="' . esc_attr__( 'Chahk: rain deity', 'archeo' ) . '"/></figure><div class="wp-block-media-text__content"><!-- wp:heading {"style":{"typography":{"fontSize":"clamp(64px, 6vw, 100px)","lineHeight":"1"},"spacing":{"margin":{"bottom":"48px"}}}} --><h2 id="chahk-raindeity" style="font-size:clamp(64px, 6vw, 100px);line-height:1;margin-bottom:48px">' . wp_kses_post( __( 'CHAHK:<br>RAIN<br>DEITY', 'archeo' ) ) . '</h2>


On the wp:heading, we could use "textTransform":"uppercase" to automatically make the text uppercase here.

Done in 2f4587c

kjellr · 2022-02-08T13:51:30Z

The jpg is pretty big at 1.29MB, do we normally compress these before bundling them?

Yes, but let's do them all at once after all the patterns are in the theme. If you open an issue and assign it to me, I'll take care of it. 👍

kjellr · 2022-02-08T13:55:17Z

Let's replace the image with this transparent one for now:

https://cloudup.com/cyioESVKP7K

kjellr · 2022-02-08T13:55:44Z

Hmm.. @kjellr adjusted the comps for us so this would work nicely, maybe we should change them back to the other way around?

Oh weird. Does the Media & Text block always place the image on the top?

scruffian · 2022-02-08T14:07:54Z

Oh weird. Does the Media & Text block always place the image on the top?

Yes, unless you disable "Stack on mobile"

kjellr · 2022-02-08T14:12:23Z

Ah, ok. I wouldn't change that with CSS then. But if that's the case, let's switch back to the version that used right-aligned text:

…ing, reduce bottom padding.

scruffian · 2022-02-08T14:49:45Z

This is ready for another review

kjellr · 2022-02-08T15:06:13Z

Just pushed a couple small updates:

I removed the transparent pixels around the image.
I set the group parent to inherit default layout (I think this was probably left out by mistake?).

I think this should be ready to go.

MaggieCabrera changed the title ~~created pattern~~ Archeo: Image with headline and description pattern Feb 7, 2022

MaggieCabrera self-assigned this Feb 7, 2022

MaggieCabrera requested a review from a team February 7, 2022 10:12

MaggieCabrera mentioned this pull request Feb 7, 2022

Archeo: Block patterns #5429

Closed

9 tasks

jffng reviewed Feb 7, 2022

View reviewed changes

jffng requested a review from kjellr February 7, 2022 17:50

jffng reviewed Feb 7, 2022

View reviewed changes

mikachan mentioned this pull request Feb 8, 2022

Archeo: Add Layered images with headline pattern #5468

Merged

mikachan reviewed Feb 8, 2022

View reviewed changes

MaggieCabrera and others added 6 commits February 8, 2022 14:49

created pattern

fb0eb4c

Use a responsive font size for the heading, reduce margin on the head…

1ac1e29

…ing, reduce bottom padding.

Move text decoration to an attribute

a5e222e

Update image to transparent version

1827f03

move text to the right

3f568fa

fix broken block markup

2c095d7

scruffian force-pushed the archeo-pattern-image-headline branch from cc2864a to 2c095d7 Compare February 8, 2022 14:49

kjellr added 3 commits February 8, 2022 10:03

Remove transparent pixels in the image.

a65ae2b

Inherit layout

58e57f8

Merge branch 'trunk' into archeo-pattern-image-headline

e6358d4

scruffian merged commit d18fdd1 into trunk Feb 8, 2022

scruffian deleted the archeo-pattern-image-headline branch February 8, 2022 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Archeo: Image with headline and description pattern #5467

Archeo: Image with headline and description pattern #5467

MaggieCabrera commented Feb 7, 2022 •

edited

Loading

jffng left a comment

jffng Feb 7, 2022

jffng commented Feb 7, 2022

jffng Feb 7, 2022

mikachan Feb 8, 2022

mikachan commented Feb 8, 2022

mikachan left a comment

mikachan Feb 8, 2022

scruffian Feb 8, 2022

kjellr commented Feb 8, 2022

kjellr commented Feb 8, 2022

kjellr commented Feb 8, 2022

scruffian commented Feb 8, 2022

kjellr commented Feb 8, 2022

scruffian commented Feb 8, 2022

kjellr commented Feb 8, 2022

Archeo: Image with headline and description pattern #5467

Archeo: Image with headline and description pattern #5467

Conversation

MaggieCabrera commented Feb 7, 2022 • edited Loading

Changes proposed in this Pull Request:

Related issue(s):

jffng left a comment

Choose a reason for hiding this comment

jffng Feb 7, 2022

Choose a reason for hiding this comment

jffng commented Feb 7, 2022

jffng Feb 7, 2022

Choose a reason for hiding this comment

mikachan Feb 8, 2022

Choose a reason for hiding this comment

mikachan commented Feb 8, 2022

mikachan left a comment

Choose a reason for hiding this comment

mikachan Feb 8, 2022

Choose a reason for hiding this comment

scruffian Feb 8, 2022

Choose a reason for hiding this comment

kjellr commented Feb 8, 2022

kjellr commented Feb 8, 2022

kjellr commented Feb 8, 2022

scruffian commented Feb 8, 2022

kjellr commented Feb 8, 2022

scruffian commented Feb 8, 2022

kjellr commented Feb 8, 2022

MaggieCabrera commented Feb 7, 2022 •

edited

Loading