Chore: Styles Gutenberg core image block #802

derekshirk · 2020-06-18T23:21:33Z

Overview

This pull request adds a few small style adjustments to Gutenberg image blocks to improve layout and alignment for WP image blocks.

Removes margin-bottom from image blocks
Fixes issue with smaller/resized images with center alignment

Additionally, This PR now adds CSS support for Gutenberg wide and full images.

Screenshots

Before alignment fix

After alignment fix

Testing

Review changes
Check code comments, make sure they are clear, concise, explain the "why" of what they are annotating and above all that the changes they are introducing are sensible.

See #710

changeset-bot · 2020-06-18T23:21:36Z

🦋 Changeset is good to go

Latest commit: 361cae5

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

…atterns into chore/gutenberg-core-image-block

derekshirk · 2020-06-22T23:54:44Z

After I opened this PR up for review, I realized that this PR did not include the changes I intended to add to the core/image documentation about how u-pull-inline utility classes can be used to extend the left and right edges of an image outside of the parent prose container.

I plan to update that before merging.

derekshirk · 2020-06-23T16:52:41Z

src/vendor/wordpress/gutenberg.stories.mdx

+`u-pull-inline-{amount}` class names from our [negative
+margin](/?path=/docs/utilities-spacing--negative-margin) utility classes can
+be added to the additional css section of the image block to extend the left and
+right edges of images beyond their parent prose container.


I've updated this section of documentation and re-requesting review.

I'm surprised to see the direction to use utility classes. Is it possible for us to add opinionated defaults for the breakout alignment options I see in the Gutenberg demo ("wide width," "full width")? I'd love to get these common things out of our post drafts and into our code.

Is it possible for us to add opinionated defaults for the breakout alignment options I see in the Gutenberg demo

I will update this PR to account for the "wide width" and "full width" settings.

@tylersticka I know you had a busy day, so no need to respond to this right away.

Part of my confusion was that my image block editor options looked different than your screenshot above from your Gutenberg demo. In my local development environment, I'm using the version of Gutenberg that ships with WordPress and my 'alignment' options only included: left, center and right options.

As far as I can tell, this option needs to be enabled via add_theme_support
https://gogutenberg.com/blocks/image/

Which appears to work...

Unless I'm missing something, my plan is to submit a PR to the WP repo to allow for these "advanced alignment" options as well.

…de image alignment

…atterns into chore/gutenberg-core-image-block

derekshirk · 2020-08-12T20:58:07Z

@tylersticka

You might consider adding knobs for this to toggle the various classes, which might make testing easier?

Excellent idea!

A few notes:

I'm still a little "green" when it comes to setting up knobs in Storybook, so I imagine my changes will elicit some additional feedback.

The way the story was configured prior looked like this:

<Preview>
  <Story name="core/image">
    {`<figure class="wp-block-image size-large is-style-default">
        <img
          src="/media/MtBlanc1.jpg"
          alt="Mont Blanc"
          class="wp-image-130"
          srcset="
            /media/MtBlanc1.jpg         500w,
            /media/MtBlanc1-300x225.jpg 300w
          "
          sizes="(max-width: 500px) 100vw, 500px"
        />
        <figcaption>Mont Blanc appears—still, snowy, and serene.</figcaption>
      </figure>`}
  </Story>
</Preview>

I wasn't sure how to add knobs to this format, so I followed some examples I observed elsewhere and reformatted the story like this:

<Preview>
  <Story name="core/image">
    {blockImageDemo({
      class: select('alignment class', alignmentClasses, null),
    })}
  </Story>
</Preview>

I am curious about the legitimacy of this approach and possibly learning better/alternate ways to configure this.

Regarding the select knob...I ended up adding the options like this:

const alignmentClasses = {
  None: null,
  Left: 'alignleft',
  Center: 'aligncenter',
  Right: 'alignright',
  Full: 'alignfull',
  Wide: 'alignwide',
};

I wonder if their is a more efficient way to accomplish this.

I assumed it would help demonstrate the effects of alignment classes, if I added surrounding placeholder text to the block/image story but wanted to confirm if this was desirable or not.
I added some conditional logic to src/vendor/wordpress/demo/image.twig that might seem strange (6200fed). I did this because WP actually modifies the markup of the image block it outputs depending the selected alignment classes. Feedback welcome!

tylersticka

Works well, some comments inline

tylersticka · 2020-08-12T21:46:37Z

src/vendor/wordpress/_wordpress.scss

+  position: relative;
+
+  /**
+   * 1. Setting a width of `100%` prevents resized and centered images from


Nit: Comment numbering isn't necessary when there is only one line in the block.

src/vendor/wordpress/_wordpress.scss

tylersticka · 2020-08-12T21:48:46Z

src/vendor/wordpress/_wordpress.scss

+      left: calc(12vw / -2); /* 1 */
+      margin-left: auto;
+      margin-right: auto;
+      width: calc(100% + 12vw); /* 1 */


Would this accomplish the same thing?

Suggested change

left: calc(12vw / -2); /* 1 */

margin-left: auto;

margin-right: auto;

width: calc(100% + 12vw); /* 1 */

margin-left: -6vw; /* 1 */

margin-right: -6vw; /* 1 */

It does! I like it, much simpler. I am introducing these changes manually.

tylersticka · 2020-08-12T21:51:44Z

src/vendor/wordpress/gutenberg.stories.mdx

-<Meta title="Vendor/WordPress/Gutenberg" />
+import blockImageDemo from './demo/image.twig';
+const alignmentClasses = {
+  None: null,


Storybook select values are converted to strings no matter what they are, so (annoyingly) null is the same as passing in a string called 'null'. You can pass in an empty string, though, (''), which should have the desired effect.

(It's times like these I wish knobs were better documented... but they've been replaced by a more elegant solution in the latest Storybook release, so that's nice!)

Thank you, that was extremely helpful. I had originally started with an empty string, but ended up mimicking what I saw in the documentation I was reading, which used null. 🙃

tylersticka · 2020-08-12T21:53:46Z

src/vendor/wordpress/gutenberg.stories.mdx

-        <figcaption>Mont Blanc appears—still, snowy, and serene.</figcaption>
-      </figure>`}
+    {blockImageDemo({
+      class: select('alignment class', alignmentClasses, null),


I would either shorten this to "alignment," or show the full class names (alignright instead of Right). As is, it says "alignment class" but it's not actually showing the class.

Good point! Excellent feedback.

tylersticka · 2020-08-12T21:55:11Z

src/vendor/wordpress/demo/image.twig

@@ -0,0 +1,30 @@
+<div
+  class="demo-container"


Consider using the Rhythm object pattern for this, especially since your styles account for it.

tylersticka

Working well, but two issues still to resolve:

I notice if I change from the "None" alignment option to "Center," the amount of whitespace below the example increases but nothing else does.
Because this PR adds a feature, you need to make sure the addition is part of our changelog. Instructions are in the contribution guide: https://github.com/cloudfour/cloudfour.com-patterns/blob/v-next/CONTRIBUTING.md#changelog-entries

…ment options (none/center).

derekshirk · 2020-08-13T23:58:06Z

Hi @tylersticka,

I notice if I change from the "None" alignment option to "Center," the amount of whitespace below the example increases but nothing else does.

I want to make sure I understand expectations here correctly.

1. Whitespace question

I can't determine if you are suggesting that the amount of whitespace should change or not. I had originally assumed you were suggesting that it should not, so I pushed one possible "solution" (3ed4aae) to "remedy" that. However, now I'm not so sure!

Example of update introducing no change in whitespace

If I compare behavior of c4staging.wpengine.com/ master branch I notice the white space does change when switching between these two alignment options:

Note the classic editor will apply a class of alignnone while the block editor does not. alignnone does not appear to have any styles associated with it.

2. Alignment question

I also want to make sure I understand the second part of your comment:

but nothing else does.

I interpret this as, "when 'None' is selected and the image size is reduced it should not maintain centered alignment". It appears that "None" and "Center" currently behave similarly when reducing the image size due to some styles defined in our base CSS.

/src/base/_defaults.scss

/*
 * ...
 * 2. Center content
 * ...
 */

figure {
  ...

  & > * {
    margin-left: auto; /* 2 */
    margin-right: auto; /* 2 */
  }

  ...

If my interpretation is correct, is it more desirable to override the existing <figure> styles defined in _defaults.scss within our vendor _wordpress.scss (only for .wp-block-image elements), so that when no alignment class is selected the resulting behavior is like this:

An alternate approach might require rethinking how the base <figure> styles are written but centering <figure> contents by default seemed smart to me.

Thanks!

tylersticka · 2020-08-14T00:22:41Z

I can't determine if you are suggesting that the amount of whitespace should change or not. I had originally assumed you were suggesting that it should not, so I pushed one possible "solution" (3ed4aae) to "remedy" that. However, now I'm not so sure!

I couldn't see a reason why one would have whitespace and the other would not. The issue is the (seemingly unintentional) inconsistency, not the presence of or lack of whitespace.

Was the whitespace intentional, or an oversight? If it was not intentional, and there isn't a design justification for it, then we should remove it. If it was intentional but was omitted from the default, then we should add it.

Does that make sense?

I also want to make sure I understand the second part of your comment:

but nothing else does.

This isn't a second part, I just meant that only aligncenter adds whitespace. It's just the inconsistency.

If you intended for aligncenter to have more whitespace below it, then we should have added whitespace consistency. If you did not intend for aligncenter to have more whitespace below it, then you were correct in removing it.

tylersticka · 2020-08-14T00:24:48Z

To summarize:

Did you intend for aligncenter to have more whitespace below it?
- Yes: I'd like to know why. We can't just have inconsistency without a justification.
- No: Okay, is it an improvement (or "happy accident")?
  - Yes: Then we should add it to the other options as well.
  - No: Then it was a good idea to remove it.

derekshirk · 2020-08-15T00:24:13Z

Did you intend for aligncenter to have more whitespace below it?

Based on my observations on cloudfour.com I had thought the additional whitespace was intentional. So my only "intent" at the time was to mimic what I thought was expected, due to the display: block; property included with WordPress's aligncenter class. This is by no means a justification.

No: Okay, is it an improvement (or "happy accident")?
→ Yes: Then we should add it to the other options as well.
→ No: Then it was a good idea to remove it.

No, I don't think so. I think the additional whitespace throws off the vertical rhythm applied by our Rhythm object pattern.

Happy to discuss further.

derekshirk · 2020-08-17T17:03:55Z

Hi @tylersticka,

I added a changeset (69c1af7) and it dismissed your approval.

adds alignment styles for wp-block-image

2648968

derekshirk mentioned this pull request Jun 18, 2020

Chore: Adds embed object mixin #754

Merged

derekshirk self-assigned this Jun 18, 2020

derekshirk added 4 commits June 22, 2020 12:00

updates WP image block layout styles

7f10dc8

Merge branch 'v-next' of https://github.com/cloudfour/cloudfour.com-p…

5885712

…atterns into chore/gutenberg-core-image-block

updates comment

8d31845

Merge branch 'v-next' of https://github.com/cloudfour/cloudfour.com-p…

18649d7

…atterns into chore/gutenberg-core-image-block

derekshirk marked this pull request as ready for review June 22, 2020 23:22

derekshirk requested review from tylersticka and removed request for tylersticka June 22, 2020 23:22

derekshirk changed the title ~~Chore: Styles Gutenberg core image block~~ [DRAFT] Chore: Styles Gutenberg core image block Jun 22, 2020

tylersticka previously approved these changes Jun 22, 2020

View reviewed changes

derekshirk changed the title ~~[DRAFT] Chore: Styles Gutenberg core image block~~ Chore: Styles Gutenberg core image block Jun 22, 2020

updates image block documentation

18e58d9

derekshirk dismissed tylersticka’s stale review via 18e58d9 June 23, 2020 16:40

derekshirk requested a review from tylersticka June 23, 2020 16:51

derekshirk commented Jun 23, 2020

View reviewed changes

tylersticka removed their request for review June 24, 2020 16:30

calebeby mentioned this pull request Jul 15, 2020

Improve GitHub Actions Performance #848

Closed

derekshirk and others added 9 commits August 6, 2020 11:39

Merge branch 'v-next' into chore/gutenberg-core-image-block

87df45a

Merge branch 'v-next' into chore/gutenberg-core-image-block

c45bab6

removes documentation that went against the intended direction for wi…

4052b08

…de image alignment

adds styles for alignwide and alignwide selectors

f8a973b

improves comment quality

4ba4505

updates nested selectors

46deaa7

simplifies CSS

7c068d9

linting updates

d2ee872

Merge branch 'v-next' of https://github.com/cloudfour/cloudfour.com-p…

19d0dd7

…atterns into chore/gutenberg-core-image-block

derekshirk requested a review from tylersticka August 12, 2020 20:58

tylersticka reviewed Aug 12, 2020

View reviewed changes

derekshirk added 6 commits August 12, 2020 15:33

removes numbered comment format - PR feedback

ea4ac4c

adds comment & reference link - PR feedback

a190909

simplifies CSS properties for alignwide selector

7881b37

replaces null with empty string. Adjusts demo markup accordingly

725639b

updates knob select label - PR feedback

4b3611f

updates demo with o-rhythm class

9a6a973

derekshirk requested a review from tylersticka August 12, 2020 23:17

Merge branch 'v-next' into chore/gutenberg-core-image-block

5d884d5

tylersticka reviewed Aug 12, 2020

View reviewed changes

PR feedback - eliminate unnecessary margin shift when switching align…

3ed4aae

…ment options (none/center).

Merge branch 'v-next' into chore/gutenberg-core-image-block

ab8b1a9

tylersticka previously approved these changes Aug 17, 2020

View reviewed changes

derekshirk and others added 2 commits August 17, 2020 09:36

Merge branch 'v-next' into chore/gutenberg-core-image-block

794258b

adds changset

69c1af7

derekshirk dismissed tylersticka’s stale review via 69c1af7 August 17, 2020 16:46

Merge branch 'v-next' into chore/gutenberg-core-image-block

361cae5

derekshirk requested a review from tylersticka August 17, 2020 17:03

tylersticka approved these changes Aug 17, 2020

View reviewed changes

derekshirk merged commit eeba7a8 into v-next Aug 17, 2020

derekshirk deleted the chore/gutenberg-core-image-block branch August 17, 2020 17:09

derekshirk mentioned this pull request Aug 18, 2020

Chore: improve default Gutenberg embed block presentation #909

Closed

tylersticka mentioned this pull request Jun 23, 2022

WordPress alignwide utility width should be based on size.$max-width-spread token #1872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Styles Gutenberg core image block #802

Chore: Styles Gutenberg core image block #802

derekshirk commented Jun 18, 2020 •

edited

Loading

changeset-bot bot commented Jun 18, 2020 •

edited

Loading

derekshirk commented Jun 22, 2020

derekshirk Jun 23, 2020

tylersticka Jun 23, 2020

derekshirk Jun 23, 2020 •

edited

Loading

derekshirk Aug 6, 2020 •

edited

Loading

derekshirk commented Aug 12, 2020 •

edited

Loading

tylersticka left a comment

tylersticka Aug 12, 2020

tylersticka Aug 12, 2020

derekshirk Aug 12, 2020 •

edited

Loading

tylersticka Aug 12, 2020

derekshirk Aug 12, 2020

tylersticka Aug 12, 2020

derekshirk Aug 12, 2020

tylersticka Aug 12, 2020

tylersticka left a comment •

edited

Loading

derekshirk commented Aug 13, 2020

tylersticka commented Aug 14, 2020

tylersticka commented Aug 14, 2020 •

edited

Loading

derekshirk commented Aug 15, 2020

derekshirk commented Aug 17, 2020

Chore: Styles Gutenberg core image block #802

Chore: Styles Gutenberg core image block #802

Conversation

derekshirk commented Jun 18, 2020 • edited Loading

Overview

Screenshots

Before alignment fix

After alignment fix

Testing

changeset-bot bot commented Jun 18, 2020 • edited Loading

🦋 Changeset is good to go

derekshirk commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekshirk Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

derekshirk Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

derekshirk commented Aug 12, 2020 • edited Loading

tylersticka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekshirk Aug 12, 2020 • 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

Choose a reason for hiding this comment

tylersticka left a comment • edited Loading

Choose a reason for hiding this comment

derekshirk commented Aug 13, 2020

1. Whitespace question

2. Alignment question

tylersticka commented Aug 14, 2020

tylersticka commented Aug 14, 2020 • edited Loading

derekshirk commented Aug 15, 2020

derekshirk commented Aug 17, 2020

derekshirk commented Jun 18, 2020 •

edited

Loading

changeset-bot bot commented Jun 18, 2020 •

edited

Loading

derekshirk Jun 23, 2020 •

edited

Loading

derekshirk Aug 6, 2020 •

edited

Loading

derekshirk commented Aug 12, 2020 •

edited

Loading

derekshirk Aug 12, 2020 •

edited

Loading

tylersticka left a comment •

edited

Loading

tylersticka commented Aug 14, 2020 •

edited

Loading