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

Media & Text Block: Add vertical alignment options #13989

Merged

Conversation

frontdevde
Copy link
Contributor

@frontdevde frontdevde commented Feb 21, 2019

Description

This PR aims to add vertical alignment options to the Media & Text Block.

Important Note:

Instead of the initially custom vertical alignment implementation, this PR now builds upon the great work that @getdave has been doing to streamline this functionality by creating the BlockVerticalAlignmentToolbar component. Thus this PR will remain in Draft stage until the PR #13899, which includes the BlockVerticalAlignmentToolbar component, has been merged.

Screenshots

media-text-block-valign

Todo

How has this been tested?

  • Manually verified ability to vertically align content to top/middle/bottom
  • Manually verified vertical alignment preview via block editor in editor mode
  • Manually verified save output on website aligns correctly in various permutations of alignment
  • Manually verified enhancement doesn't require migration path (ie: deprecation) when upgrading from old form of the Block to the one in this PR (you can do this by switching to master adding the old Block, publishing/updating the post, then switching back to this PR and testing)
  • Manually tested how valign behaves in Responsive (ie: non-desktop) context

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@frontdevde frontdevde added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Block] Media & Text Affects the Media & Text Block labels Feb 21, 2019
@frontdevde frontdevde added this to the 5.3 (Gutenberg) milestone Feb 21, 2019
@frontdevde frontdevde self-assigned this Feb 21, 2019
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this.

Looks great and works well. Some minor nit picks but nothing critical.

One thing it might be making it even more explicit in the PR description that feedback for anything other than the Media + Text Block (should be provided on my PR else we'll get in a real mess!).

@kjellr
Copy link
Contributor

kjellr commented Feb 21, 2019

Looks good so far, @frontdevde.

I do have questions about a default state — if we're center aligning things by default, we should probably have that option selected in the toolbar.


Another observation is that the additional margins that exist on these blocks currently mean that top + bottom aligned blocks will never actually line up with top/bottom of the image next to them. 😕

This is really obvious when the InnerBlock isn't being hovered over. In this case, it just looks like the text is being moved slightly up and down by the buttons, rather than being top/bottom aligned.

media-text-alignment

I'm not totally sure what we can do about this, but it's possible we can trim some of those margins a bit in this case. @jasmussen any thoughts on that?

@frontdevde
Copy link
Contributor Author

Thanks for the feedback, @kjellr 👍

I do have questions about a default state — if we're center aligning things by default, we should probably have that option selected in the toolbar.

I've had the same question myself before in the context of @getdave's PR #13899 that introduces the vertical alignment toolbar component. I'm quoting the reply I got at the time, in the context of said PR:

"Essentially we should look to avoid introducing deprecations where possible. By enforcing a vertical alignment to top for Columns we may overide styles provided by existing Themes which won’t expect us to introduce an editor driven CSS alignment. If we default to undefined then we retain existing functionality for Columns. The user can then choose to activate an alignment overide as required. This approach also avoids us having to write an explicit Block deprecation."

--

Another observation is that the additional margins that exist on these blocks currently mean that top + bottom aligned blocks will never actually line up with top/bottom of the image next to them.

Yes, noticed that as well. Happy for suggestions!

@jasmussen
Copy link
Contributor

Thanks for the ping.

Regarding having the button already pre-selected, I think it's okay for it not to. To me this is not so different from us having a Text align left button, but it not being selected despite most text being left aligned. It is also my understanding that the fewer actual properties we set on blocks, the more a theme can style the defaults.

Regarding the innerblocks additional margin — yeah, this is a challenge. Whenever an innerblocks container is added, additional margin is added as well. This is also one of the challenges we have had to deal with for the columns block, where the singular column blocks add unnecessary margin. We've manually zeroed that out, which was a great headache, and kind of a mess.

It seems like the solution to that might involve #7694. Or it might involve a slight refactor of how block paddings and margins are painted. That's likely a big one, and one to look at separately. Something in the vein of #8350 but good.

@getdave
Copy link
Contributor

getdave commented Feb 22, 2019

Thanks for the feedback on this.

fewer actual properties we set on blocks, the more a theme can style the defaults.

This is the key point here. We should avoid setting defaults that break existing Blocks backwards compatibility.

innerblocks additional margin

This feels like it could be something that needs to be addressed to ship this. @frontdevde might you be able to do some investigative research and advise on the amount of work that might be involved here? What's the scope of the problem? Then we can determine whether it can be realisitically be addressed within this PR.

@jasmussen
Copy link
Contributor

I dove in and took a look. It seems #8350 is definitely the concept to revisit.

The thing right now is that every block is born with a top and bottom margin. That includes inner blocks. Normally content has margins applied on its own. I.e. a paragraph has a top and bottom margin. Those margins even collapse according to obtuse rules.

But the Columns block should not have a top/bottom margin, neither should the column block that nests inside it. Only the content inside should have its own intrinsic margins.

As mentioned, the columns block currently zeroes out and overrides this global block margin. Which is hacky. Any fix for this PR would likely involve the same. It seems revisiting #8350 separately is a best bet. I can probably take a look next week. But if anyone else feels adventurous, the margin to refactor out lives here: https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/block-list/style.scss#L60 — I am almost convinced that's the margin that makes the innerblocks have margins they shouldn't have. But be aware this margin affects a lot, so it's not a simple thing to fix.

@marekhrabe
Copy link
Contributor

The functionality looks really solid to me. Extra margins now noticeable with the top/bottom alignments.

I see some extra files in here (like icons.js) but I assume it's just a matter of rebasing again on top of the #13899, right?

@frontdevde
Copy link
Contributor Author

I see some extra files in here (like icons.js) but I assume it's just a matter of rebasing again on top of the #13899, right?

That is correct! 👍

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @frontdevde thank you for this enhancements 👍 I focused my review on media & text changes as the other artifacts are already being reviewed in #13899.
Things worked well in my tests, I just left some suggestions.

packages/block-library/src/media-text/editor.scss Outdated Show resolved Hide resolved
jasmussen added a commit that referenced this pull request Mar 13, 2019
This PR is round 2 of #8350. The ultimate goal is to make it easier for themes to style the editor, and to do less CSS overriding in order to do so.

Problem: Currently, every single block is born with an intrinsic top and bottom margin. This margin matches the padding that sits between the block and the "selected block" border, plus 2px of space. This margin is the same regardless of whether the block needs a margin or not, and it is applied to nesting containers as well. In the case of the Columns block for example, that means the _column_ block (singular) has top and bottom margin, even though it shouldn't have that.

Since then a number of changes have been made to the editor to make it a good time to revisit this:

- The block outlines and toolbars have been refactored to not rely on margins at all to position themselves. These will still be painted correctly outside of the block, though they may overlap content visibly if a zero margin block is selected.
- A more solid editor style system has been introduced that makes it easier to customize the editor to look like the front-end. As a result of this, feedback around CSS specificity and having to override these margins have surfaced to a higher degree.

Proposed solution: By removing the intrinsic margin, we can re-apply it only to the blocks that actually _should_ be born with an intrinsic margin, such as paragraphs, lists, quotes ,etc.

Some discussion points that are likely to surface: where should those vanilla styles be stored? How should they be structured so that themes can easily override them? Should these _not_ be loaded if a theme provides its own editor styles? Should we leverage the cascade and store these generically in one location or should these be applied in the style.scss file for every block in the library? Given these intrinsic margins have been present since day one, can we expect plugin authors to remember to add these margins themselves for every block they make? Is there a back-compat way we provide these default margins to blocks that rely on them?

This is a try branch, in order to figure out answers to those questions. This first commit only does a few things:

- It rearranges some CSS to put things in more logical locations.
- It removes the intrinsic margins, then blanket reapplies them in that new vanilla stylesheet location with a new CSS variable.

Next commits will explore how to remove that blanket reapplication, and try to provide those vanilla styles in a per-block basis.

See also:

#13989 (comment)
https://github.com/WordPress/gutenberg/pull/8350/files
@m-e-h
Copy link
Member

m-e-h commented Mar 13, 2019

This is nice!
I actually had a request from a user the other day, to make the media-image stretch to match the full-height of the media text. Is that another button that would make sense or should be added here? ↕️

@frontdevde frontdevde force-pushed the update/media-text-block-v-align branch from 7e291fc to 1e7ebdc Compare March 13, 2019 23:52
@frontdevde frontdevde added [Status] Blocked Used to indicate that a current effort isn't able to move forward and removed Needs Design Feedback Needs general design feedback. labels Mar 13, 2019
@frontdevde
Copy link
Contributor Author

This pull request is still a work in progress.

Thus this PR will remain in Draft stage until the PR #13899, which includes the BlockVerticalAlignmentToolbar component, has been merged.

I've updated the state of this Draft PR to only show the diff to PR #13899. We'll get back to this once PR #13899 has landed. For now I've added the Status [Blocked] label.

jasmussen added a commit that referenced this pull request Mar 15, 2019
This PR is round 2 of #8350. The ultimate goal is to make it easier for themes to style the editor, and to do less CSS overriding in order to do so.

Problem: Currently, every single block is born with an intrinsic top and bottom margin. This margin matches the padding that sits between the block and the "selected block" border, plus 2px of space. This margin is the same regardless of whether the block needs a margin or not, and it is applied to nesting containers as well. In the case of the Columns block for example, that means the _column_ block (singular) has top and bottom margin, even though it shouldn't have that.

Since then a number of changes have been made to the editor to make it a good time to revisit this:

- The block outlines and toolbars have been refactored to not rely on margins at all to position themselves. These will still be painted correctly outside of the block, though they may overlap content visibly if a zero margin block is selected.
- A more solid editor style system has been introduced that makes it easier to customize the editor to look like the front-end. As a result of this, feedback around CSS specificity and having to override these margins have surfaced to a higher degree.

Proposed solution: By removing the intrinsic margin, we can re-apply it only to the blocks that actually _should_ be born with an intrinsic margin, such as paragraphs, lists, quotes ,etc.

Some discussion points that are likely to surface: where should those vanilla styles be stored? How should they be structured so that themes can easily override them? Should these _not_ be loaded if a theme provides its own editor styles? Should we leverage the cascade and store these generically in one location or should these be applied in the style.scss file for every block in the library? Given these intrinsic margins have been present since day one, can we expect plugin authors to remember to add these margins themselves for every block they make? Is there a back-compat way we provide these default margins to blocks that rely on them?

This is a try branch, in order to figure out answers to those questions. This first commit only does a few things:

- It rearranges some CSS to put things in more logical locations.
- It removes the intrinsic margins, then blanket reapplies them in that new vanilla stylesheet location with a new CSS variable.

Next commits will explore how to remove that blanket reapplication, and try to provide those vanilla styles in a per-block basis.

See also:

#13989 (comment)
https://github.com/WordPress/gutenberg/pull/8350/files
jasmussen added a commit that referenced this pull request Mar 15, 2019
This PR is round 2 of #8350. The ultimate goal is to make it easier for themes to style the editor, and to do less CSS overriding in order to do so.

Problem: Currently, every single block is born with an intrinsic top and bottom margin. This margin matches the padding that sits between the block and the "selected block" border, plus 2px of space. This margin is the same regardless of whether the block needs a margin or not, and it is applied to nesting containers as well. In the case of the Columns block for example, that means the _column_ block (singular) has top and bottom margin, even though it shouldn't have that.

Since then a number of changes have been made to the editor to make it a good time to revisit this:

- The block outlines and toolbars have been refactored to not rely on margins at all to position themselves. These will still be painted correctly outside of the block, though they may overlap content visibly if a zero margin block is selected.
- A more solid editor style system has been introduced that makes it easier to customize the editor to look like the front-end. As a result of this, feedback around CSS specificity and having to override these margins have surfaced to a higher degree.

Proposed solution: By removing the intrinsic margin, we can re-apply it only to the blocks that actually _should_ be born with an intrinsic margin, such as paragraphs, lists, quotes ,etc.

Some discussion points that are likely to surface: where should those vanilla styles be stored? How should they be structured so that themes can easily override them? Should these _not_ be loaded if a theme provides its own editor styles? Should we leverage the cascade and store these generically in one location or should these be applied in the style.scss file for every block in the library? Given these intrinsic margins have been present since day one, can we expect plugin authors to remember to add these margins themselves for every block they make? Is there a back-compat way we provide these default margins to blocks that rely on them?

This is a try branch, in order to figure out answers to those questions. This first commit only does a few things:

- It rearranges some CSS to put things in more logical locations.
- It removes the intrinsic margins, then blanket reapplies them in that new vanilla stylesheet location with a new CSS variable.

Next commits will explore how to remove that blanket reapplication, and try to provide those vanilla styles in a per-block basis.

See also:

#13989 (comment)
https://github.com/WordPress/gutenberg/pull/8350/files
@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@getdave
Copy link
Contributor

getdave commented Mar 19, 2019

@frontdevde The VAlign PR is now merged. Note that BlockVerticalAlignmentToolbar has moved to the @wordpress/block-editor package.

#13899

@m-e-h
Copy link
Member

m-e-h commented Mar 31, 2019

@mtias to quote @frontdevde here #13989 (comment)

Is the feature request you were referring to covered by PR #14445 ?

😄

@frontdevde frontdevde requested review from aduth, nerrad and ntwb as code owners April 1, 2019 22:16
@frontdevde frontdevde removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Apr 1, 2019
@frontdevde
Copy link
Contributor Author

frontdevde commented Apr 1, 2019

@frontdevde Are we still Blocked here?

@getdave No! I forgot to remove the tag, thanks for pointing that out.

Just added a fixture for a vertical alignment test. That resolves the last open comment on this one.

jasmussen added a commit that referenced this pull request Apr 2, 2019
This PR is round 2 of #8350. The ultimate goal is to make it easier for themes to style the editor, and to do less CSS overriding in order to do so.

Problem: Currently, every single block is born with an intrinsic top and bottom margin. This margin matches the padding that sits between the block and the "selected block" border, plus 2px of space. This margin is the same regardless of whether the block needs a margin or not, and it is applied to nesting containers as well. In the case of the Columns block for example, that means the _column_ block (singular) has top and bottom margin, even though it shouldn't have that.

Since then a number of changes have been made to the editor to make it a good time to revisit this:

- The block outlines and toolbars have been refactored to not rely on margins at all to position themselves. These will still be painted correctly outside of the block, though they may overlap content visibly if a zero margin block is selected.
- A more solid editor style system has been introduced that makes it easier to customize the editor to look like the front-end. As a result of this, feedback around CSS specificity and having to override these margins have surfaced to a higher degree.

Proposed solution: By removing the intrinsic margin, we can re-apply it only to the blocks that actually _should_ be born with an intrinsic margin, such as paragraphs, lists, quotes ,etc.

Some discussion points that are likely to surface: where should those vanilla styles be stored? How should they be structured so that themes can easily override them? Should these _not_ be loaded if a theme provides its own editor styles? Should we leverage the cascade and store these generically in one location or should these be applied in the style.scss file for every block in the library? Given these intrinsic margins have been present since day one, can we expect plugin authors to remember to add these margins themselves for every block they make? Is there a back-compat way we provide these default margins to blocks that rely on them?

This is a try branch, in order to figure out answers to those questions. This first commit only does a few things:

- It rearranges some CSS to put things in more logical locations.
- It removes the intrinsic margins, then blanket reapplies them in that new vanilla stylesheet location with a new CSS variable.

Next commits will explore how to remove that blanket reapplication, and try to provide those vanilla styles in a per-block basis.

See also:

#13989 (comment)
https://github.com/WordPress/gutenberg/pull/8350/files
jasmussen added a commit that referenced this pull request Apr 2, 2019
This PR is round 2 of #8350. The ultimate goal is to make it easier for themes to style the editor, and to do less CSS overriding in order to do so.

Problem: Currently, every single block is born with an intrinsic top and bottom margin. This margin matches the padding that sits between the block and the "selected block" border, plus 2px of space. This margin is the same regardless of whether the block needs a margin or not, and it is applied to nesting containers as well. In the case of the Columns block for example, that means the _column_ block (singular) has top and bottom margin, even though it shouldn't have that.

Since then a number of changes have been made to the editor to make it a good time to revisit this:

- The block outlines and toolbars have been refactored to not rely on margins at all to position themselves. These will still be painted correctly outside of the block, though they may overlap content visibly if a zero margin block is selected.
- A more solid editor style system has been introduced that makes it easier to customize the editor to look like the front-end. As a result of this, feedback around CSS specificity and having to override these margins have surfaced to a higher degree.

Proposed solution: By removing the intrinsic margin, we can re-apply it only to the blocks that actually _should_ be born with an intrinsic margin, such as paragraphs, lists, quotes ,etc.

Some discussion points that are likely to surface: where should those vanilla styles be stored? How should they be structured so that themes can easily override them? Should these _not_ be loaded if a theme provides its own editor styles? Should we leverage the cascade and store these generically in one location or should these be applied in the style.scss file for every block in the library? Given these intrinsic margins have been present since day one, can we expect plugin authors to remember to add these margins themselves for every block they make? Is there a back-compat way we provide these default margins to blocks that rely on them?

This is a try branch, in order to figure out answers to those questions. This first commit only does a few things:

- It rearranges some CSS to put things in more logical locations.
- It removes the intrinsic margins, then blanket reapplies them in that new vanilla stylesheet location with a new CSS variable.

Next commits will explore how to remove that blanket reapplication, and try to provide those vanilla styles in a per-block basis.

See also:

#13989 (comment)
https://github.com/WordPress/gutenberg/pull/8350/files
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

The code changes look good and everything worked as expected in my tests. Thank you for this improvement 👍

@frontdevde frontdevde merged commit 5aed6ae into WordPress:master Apr 3, 2019
@frontdevde frontdevde deleted the update/media-text-block-v-align branch April 3, 2019 00:19
jasmussen added a commit that referenced this pull request Apr 12, 2019
This PR is round 2 of #8350. The ultimate goal is to make it easier for themes to style the editor, and to do less CSS overriding in order to do so.

Problem: Currently, every single block is born with an intrinsic top and bottom margin. This margin matches the padding that sits between the block and the "selected block" border, plus 2px of space. This margin is the same regardless of whether the block needs a margin or not, and it is applied to nesting containers as well. In the case of the Columns block for example, that means the _column_ block (singular) has top and bottom margin, even though it shouldn't have that.

Since then a number of changes have been made to the editor to make it a good time to revisit this:

- The block outlines and toolbars have been refactored to not rely on margins at all to position themselves. These will still be painted correctly outside of the block, though they may overlap content visibly if a zero margin block is selected.
- A more solid editor style system has been introduced that makes it easier to customize the editor to look like the front-end. As a result of this, feedback around CSS specificity and having to override these margins have surfaced to a higher degree.

Proposed solution: By removing the intrinsic margin, we can re-apply it only to the blocks that actually _should_ be born with an intrinsic margin, such as paragraphs, lists, quotes ,etc.

Some discussion points that are likely to surface: where should those vanilla styles be stored? How should they be structured so that themes can easily override them? Should these _not_ be loaded if a theme provides its own editor styles? Should we leverage the cascade and store these generically in one location or should these be applied in the style.scss file for every block in the library? Given these intrinsic margins have been present since day one, can we expect plugin authors to remember to add these margins themselves for every block they make? Is there a back-compat way we provide these default margins to blocks that rely on them?

This is a try branch, in order to figure out answers to those questions. This first commit only does a few things:

- It rearranges some CSS to put things in more logical locations.
- It removes the intrinsic margins, then blanket reapplies them in that new vanilla stylesheet location with a new CSS variable.

Next commits will explore how to remove that blanket reapplication, and try to provide those vanilla styles in a per-block basis.

See also:

#13989 (comment)
https://github.com/WordPress/gutenberg/pull/8350/files
@youknowriad youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019
jasmussen added a commit that referenced this pull request Apr 17, 2019
…htly (#14407)

* Try: Remove intrinsic block margins, rely on cascade

This PR is round 2 of #8350. The ultimate goal is to make it easier for themes to style the editor, and to do less CSS overriding in order to do so.

Problem: Currently, every single block is born with an intrinsic top and bottom margin. This margin matches the padding that sits between the block and the "selected block" border, plus 2px of space. This margin is the same regardless of whether the block needs a margin or not, and it is applied to nesting containers as well. In the case of the Columns block for example, that means the _column_ block (singular) has top and bottom margin, even though it shouldn't have that.

Since then a number of changes have been made to the editor to make it a good time to revisit this:

- The block outlines and toolbars have been refactored to not rely on margins at all to position themselves. These will still be painted correctly outside of the block, though they may overlap content visibly if a zero margin block is selected.
- A more solid editor style system has been introduced that makes it easier to customize the editor to look like the front-end. As a result of this, feedback around CSS specificity and having to override these margins have surfaced to a higher degree.

Proposed solution: By removing the intrinsic margin, we can re-apply it only to the blocks that actually _should_ be born with an intrinsic margin, such as paragraphs, lists, quotes ,etc.

Some discussion points that are likely to surface: where should those vanilla styles be stored? How should they be structured so that themes can easily override them? Should these _not_ be loaded if a theme provides its own editor styles? Should we leverage the cascade and store these generically in one location or should these be applied in the style.scss file for every block in the library? Given these intrinsic margins have been present since day one, can we expect plugin authors to remember to add these margins themselves for every block they make? Is there a back-compat way we provide these default margins to blocks that rely on them?

This is a try branch, in order to figure out answers to those questions. This first commit only does a few things:

- It rearranges some CSS to put things in more logical locations.
- It removes the intrinsic margins, then blanket reapplies them in that new vanilla stylesheet location with a new CSS variable.

Next commits will explore how to remove that blanket reapplication, and try to provide those vanilla styles in a per-block basis.

See also:

#13989 (comment)
https://github.com/WordPress/gutenberg/pull/8350/files

* Try a new "Safety Margin"

This commit includes a few typo fixes, documentation cleanup, general polish.

But moreso than that, it does a few things:

- It removes as many margins as it can, now only bottom margins remain.
- It introduces a new "safety margin" rule that targets every block. More on this in a bit.

If you look over the code, you can see that it's quite a few steps forward with regards to simplifying selectors and cleaning up the code.

But due to the [data-block] selector targetting the _wrapper_ of a block, rather than where the block starts (which is usually the first encounter of a `wp-block-*` class), this margin is actually applied outside of the content of the block. Which means this aspect is only the tiniest step forward, compared to the selector it replaces, which was also outside the content of the block.

* Add top margin also.
This was referenced Apr 17, 2019
@cdnlin
Copy link

cdnlin commented Jun 3, 2019

A question from a new-ish Gutenberg user: this is exactly what I need. Is it available to us regular folks yet? Or do you have a date for its availability yet? We sure need it for the new site we're creating!

Thanks,

Linda

@noisysocks
Copy link
Member

Hi @cdnlin, this feature is currently available for sites using the Gutenberg plugin and will soon be available for all sites running WordPress 5.3.

@cdnlin
Copy link

cdnlin commented Jun 6, 2019

Hey, great news, noisysocks! Thanks so much for letting me know. 5.3 has to be getting close since we're at 5.2.1 now. Until then, appreciate the heads up about the plugin. I haven't downloaded it yet, but will now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.