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

Core Columns Block - adds ability to vertically align all the columns #13899

Merged
merged 47 commits into from Mar 19, 2019

Conversation

@getdave
Copy link
Contributor

commented Feb 15, 2019

It is not currently possible to change the vertical alignment of Columns within the Columns Block. This results in a certain degree of inflexibility.

This PR will add:

  • Ability to adjust the vertical alignment of individual Columns within the Columns Block
  • Ability to adjust the vertical alignment of all Columns via the parent Columns Block
  • New reusable toolbar component within a@wordpress/editor to allow the vertical alignment toolbar UI to be reused for other Blocks that required this functionality.

Todo

Outstanding Queries

  • Considering extracting VAlignToolbar to a reusable Component of some kind. Waiting for guidance on whether this should live in package/components yet. Complete - based on advice received in #core-editor Slack channel.

How has this been tested?

  • Manually verify ability to align all columns from parent
  • Manually verify ability to provide per column alignment settings (and these take precedence over the parent valign setting if applied)
  • Manually verify can preview valignment via block editor in editor mode
  • Manually verify save output on website aligns correctly in various permutations of alignment
  • Manually verify 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 test how valign behaves in Responsive (ie: non-desktop) context

Screenshots

screen capture on 2019-02-19 at 11-12-59

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.

Related PRs

  • #13989 Media + Text Block Vertical Alignment
  • #13822 Cover Block Nested Blocks
@michelleweber

This comment has been minimized.

Copy link

commented Feb 20, 2019

Hi! What copy needs a check here?

@gziolo gziolo added this to the 5.3 (Gutenberg) milestone Feb 20, 2019

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Hi! What copy needs a check here?

@michelleweber I think the labels for the Toolbar here https://github.com/WordPress/gutenberg/pull/13899/files#diff-2f589ecb53b1259bbae7acf5b2dd770cR20 could do with sanity check please.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

Hey @getdave! This is a lovely new feature, thanks for working on it. From a design perspective, this is looking pretty solid.

In general, my main issues were just around how difficult it is to select parent/child blocks. But that's not limited to this feature, and we're working that out in #9628 (comment). 🙂

I did find one bug: Toolbar spacing seems a bit off on mobile. I think it's related to the new icon there:

screen shot 2019-02-20 at 9 57 11 am


I also have a few small notes we should think about regarding this feature. I'm not sure there's actually any action we should take based on this, but I figured it's worth sharing.

In general, there are a number of scenarios in which this feature will have no immediate effect. It makes sense to me (and to most of us working on this I'm sure), but I do think it'll be confusing to many users. First, the vertical alignment only kicks on columns that are not the tallest column, so users will surely run into circumstances where they're clicking these controls and not seeing anything change. Take this example for instance:

no-effect

This gets confusing because visually, there is a lot of extra top/bottom space there. We've (thankfully) added this to help with block selection, but it does make things a little confusing: It looks like the blocks could move up and down within that space, even though they can't. This may be improved upon depending on the outcome of #9628 (comment).

Similarly, columns are stacked by default on mobile, and so these controls have no effect there. This may change a bit depending on the outcome of #13363, but even now I think it's still useful to include this control there.

Finally, these controls can be applied to both the parent columns block and the individual column block. Settings on the innermost block override settings on the parent columns block. Conceptually this makes sense, but it does add a bunch of complexity. I encountered some scenarios where adjusting vertical alignment on the parent block had no effect because because the inner blocks had settings already (either because they were the tallest block already, or because they had their own setting). For simplicity's sake, we might want to consider limiting this setting to one or the other.


Again, this is looking great though. I think it'll bring a lot of great new flexibility to the columns block, and I could see the component coming in handy for lots of other blocks in the future. 🎉

@marekhrabe

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

I encountered some scenarios where adjusting vertical alignment on the parent block had no effect because because the inner blocks had settings already (either because they were the tallest block already, or because they had their own setting).

So I've had a long comment composed that was playing with this idea… You summed it up better! 🙇‍♂

Are there any precedents for similar situation where we have a setting that's being inherited/overridden elsewhere in Gutenberg? I can't think of one but I'm also new with the development here.

@michelleweber

This comment has been minimized.

Copy link

commented Feb 20, 2019

@getdave, I think they're fine. From a purely visual perspective you could probably get away with losing the "vertical" because the icon communicates that, but It's probably best to keep it for accessibility purposes.

The one thing I wondered was whether "middle" should be "center," which I feel like is the term I'm used to seeing re: aligning things.

@marekhrabe

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

@michelleweber I think I've mostly seen "Middle" being used in the vertical context (as opposed to horizontal "Center"). Examples from other editors I've tried:

GDocs TinyMCE Table
screenshot 2019-02-20 at 16 42 33 screenshot 2019-02-20 at 16 46 45

getdave added some commits Feb 25, 2019

Creates dedicated files for large code blocks
Previously all code blocks were in a single large index file. This was becoming unwieldy and difficult to manage/edit. Extract each large block into it’s own file for readability and maintainability.
Updates to avoid tests depending on erroneous _id prop
Previously the `_id` prop was included purely to make testing easier (more resilient). On reflection introducing code just for tests was a bad idea. Now depends on string comparison which is an acceptable compromise.

Resolves #13899 (comment)
Removes usage of unused attribute introduced as experiment
This attribute was introduced as part of experimenting in e759315 and never removed. It’s unused and has been removed.
Fixes to avoid mutation in render method
Previously in order to reset the Parent’s alignment when one of the child Columns set it’s own alignment, the render method of the Parent Columns mutated the attributes causing a re-render. To avoid this we now reset the Parent from the Child Column whose alignment changed.

Addresses #13899 (comment)
Restores pass through click behaviour on single Column Block
As discussed [here](#13899 (comment)) selection of parent/child Blocks is currently being worked on elsewhere. Therefore to get the vertical-alignment shipped, this commit reverts the change that made Columns individually selectable. Now reverts to original behaviour of only being selectable via the Block Inspector (or keyboard). This assumes that future PRs will land better Parent/Child selection behaviour. Also removes unwanted space within the Columns themselves.
@marekhrabe

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

I just tested the latest version now. The functionality works well for me, there is no extra whitespace and only child blocks and main Columns are selectable in the visual editor. Individual Column is accessible only through the block navigation menu at the top.

To me, this sounds like a good first iteration of the feature. Some parts are more hidden and "power user" but considering how deep those related issues are, I echo we should handle that separately later while having this PR already merged.

getdave added some commits Mar 19, 2019

Updates to move VAlign Toolbar into Block Editor package
Previously the editor package container the toolbar. Now the new Block Editor should be the new home for the toolbar

@getdave getdave force-pushed the getdave:add/core-columns-valignment branch from bc4d83d to c8713f5 Mar 19, 2019

@getdave getdave merged commit c34d6b7 into WordPress:master Mar 19, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

🚢 Enhancement now merged. Thanks to all who took the time to contribute!

const { getBlockRootClientId } = select( 'core/editor' );

return {
parentColumsBlockClientId: getBlockRootClientId( clientId ),

This comment has been minimized.

Copy link
@aduth

aduth Mar 19, 2019

Member

Noting:

  • Typo on "Colums" ("Columns")
  • getBlockRootClientId is a very non-performant selector in its current form. Ideally this shouldn't impact you in implementing this, but unfortunately it might. Options here may be to either improve the performance of (at worst, memoize) the selector, or pass through the selector, not the selector result, to be called directly in the callback of updateAlignment when it's needed.

This comment has been minimized.

Copy link
@aduth

aduth Apr 30, 2019

Member
  • getBlockRootClientId is a very non-performant selector in its current form. Ideally this shouldn't impact you in implementing this, but unfortunately it might. Options here may be to either improve the performance of (at worst, memoize) the selector, or pass through the selector, not the selector result, to be called directly in the callback of updateAlignment when it's needed.

Revisiting this since I'm encountering it again: Since it's only used in the withDispatch callback, another (preferred) option would be to leverage the third argument of withDispatch (registry) to select the necessary data on-demand only when necessary. I suspect this would provide a significant performance improvement for posts containing a columns block.

See: #11851

For what it's worth, I'm refactoring this in an unrelated effort, so it may not require any action on your part.

* Selects the child column Blocks for this parent Column
*/
withSelect( ( select, { clientId } ) => {
const { getBlocksByClientId } = select( 'core/editor' );

This comment has been minimized.

Copy link
@aduth

aduth Mar 19, 2019

Member

These selectors have all been effectively deprecated from core/editor, though they're not logged as such. We should still aim to select from core/block-editor instead.

const { getBlocksByClientId } = select( 'core/editor' );

return {
childColumns: getBlocksByClientId( clientId )[ 0 ].innerBlocks,

This comment has been minimized.

Copy link
@aduth

aduth Mar 19, 2019

Member

There's a few levels of indirection here to get to the innerBlocks. You could have called getBlock to get the singular block by clientId, but actually think it may have been better to call:

getBlocks( clientId )

...where the clientId argument of the getBlocks selector signifies the "root" (i.e. parent) from which to select inner blocks.

https://wordpress.org/gutenberg/handbook/designers-developers/developers/data/data-core-block-editor/#getblocks

} );

// Reset Parent Columns Block
dispatch( 'core/editor' ).updateBlockAttributes( parentColumsBlockClientId, {

This comment has been minimized.

Copy link
@aduth

aduth Mar 19, 2019

Member

Is it necessary to update the block if it's not guaranteed that anything will change (i.e. what about if the parent block had no vertical alignment)?

mkevins added a commit to mkevins/gutenberg that referenced this pull request Mar 26, 2019

Core Columns Block - adds ability to vertically align all the columns (
…WordPress#13899)

* Adds ability to valign all the columns via parent

Proof of concept demonstrating ability to change the valignment of columns with a parent column Block

* Adds ability to v-align individual Columns

* Adds ability to select individual Column Blocks to access Block Toolbar

Previously the ability to select an individual Column Block was disabled via CSS. This was because the Column itself was just a passthrough element with no UI or Tools of its own. Now we’ve introduced the v-align toolbar on the Column the Block needs to be selectable.

* Adds Material UI icons

@thomasguillot recommended using Material UI icons. Added dedicated icons file and implemented.

* Updates to add i18n to text strings

Ensure all text strings are translatable. Also reuse icon defs in parent Columns Block alignment settings

* Extracts reusable VAlignToolbar to DRY up implementation

* Fixes alignment attribute not updating

* Updates Toolbar API to observe Single Responsibility

Previously Toolbar was too aware of its context and assumed responsibility for updating parent Block’s attributes via setAttributes prop. Revised to utilise callback prop to propagate change events up to the parent. Now upto the consuming component to observe the change and handle it as required. This makes the VAlignToolbar easy to reuse in other locations.

Refactor Blocks to utilise new pattern.

* Extract deprecations for readability

This prepares deprecations for adding a new deprecation to handle the addition of the `verticalAlignment` attribute.

* Updates valignment default to no alignment specified

Previously we enforced a “top” valign on all columns. This seemed heavy handed given that Themes might wish to control this. Now we default to no valignment and allow user to optionally choose to specifiy a valignment.

* Adds ability to toggle alignment settings on and off

Previously once alignment had been set there was no way to disable it. Added ability to toggle each setting on and off.

* Adds editor preview of vertical alignment

* Adds reusable valign toolbar to editor package components

* Updates Columns Block to utilise new editor package toolbar component

Previously we utilised a bespoke toolbar for the Block. As this toolbar is also required in other components determined best to extract to a resuable component within the @wordpress/editor package.

Updates Block to utilise this new component. Removes old bespoke component.

See Slack convo: https://wordpress.slack.com/archives/C02QB2JS7/p1550657882430900

* Adds documentation to the BlockVerticalAlignmentToolbar

* Updates to use simplified arrow function syntax

* Fixes deprecation save definition back to original version

Previously the save definition of the deprecation was accidentally altered during testing. Restoring to the current state of `master` to ensure that we haven’t broken the existing deprecation when moving this block to a seperate file.

* Updates known classes to use simplified classnames util style

Resolves

* WordPress#13899 (comment)
* WordPress#13899 (comment)
* WordPress#13899 (comment)

* remove extraneous file with icons

* Fixes icon vars to use standard coding style

Resolves WordPress#13899 (comment)

* Updates test to use `it` instead of `test`

Resolves WordPress#13899 (comment)

* Fix spelling and grammatical errors

Resolves

* WordPress#13899 (comment)
* WordPress#13899 (comment)

* Removes duplicate dependencies comment

Resolves WordPress#13899 (comment)

* Updates code readability and self documentation

Previously the tenary to determine which icon to show when in “collapsed” mode wasn’t easy to comprehend.

Update to add clear variable names to make it more explicit about what happens when there is/isn’t a active alignment value set.

This address the concern raised in WordPress#13899 (comment) but looks to prefer code readability over absolutely succinct code.

* Consolidates selectors for improve readability/comprehension

Prevously nested of selectors was getting a little confusing. Pulled onto a single line to this unit of code in a single location.

Resolves WordPress#13899 (comment)

* Updates to consolidate similar CSS rules

Resolves WordPress#13899 (comment)

* Fixes spelling error

Resolves WordPress#13899 (comment)

* Removes unecessary type checking in favour of coercion rules

Previously we were explicitly checking the type of the `verticalAlignment` attr with `isNil()`. However this only checks for `undefined` or `null`. Other falsey values would have passed here which is undesirable. As `undefined` and `null` are both coerced to falsey we can remove the dep on `isNil` and simplify.

Resolves WordPress#13899 (comment)

* update icon sizes to match others in Toolbar

* Fixes bottom alignment on front end

Specificity overide to ensure margin is applied and preserved on last child to ensure that when columns are aligned to bottom they are are flush with each other

Resolves WordPress#13899 (comment)

* Fix failing e2e testing due to additional of toolbar

e2e tests were failing due to introduction of valign toolbar meaning that number of tab stops needed to be increased by 3 (the number of items in the new toolbar).

Also uncovered a bug with Pupeteer not handling Cmd + A to “Select All” on a Mac. This is a known issue (GoogleChrome/puppeteer#1313
) and it is now worked around using arrows and backspace.

* Fixes toolbar snapshot unit test

* Adds force update of child columns when parent setting changes

Previously it was possible to have a setting on the parent and individual settings on the individual columns. However the parent would still show an alignment setting even though not all of the child columns had that alignment.

Now if the parent has an alignment setting it will be auto applied to the state of the child columns directly. If a child column changes it’s individual alignment to something which does not match the parent’s alignment setting then the parent’s alignment setting is removed. However the individual column alignments still stay “as is” because each individual column has the valign setting directly rather than pseudo inheried from the parent via CSS.

Addresses: WordPress#13899 (comment)

* Updates changelog with `BlockVerticalAlignmentToolbar` feature

* Adds valign feature to changelog for Block Library

* Creates dedicated files for large code blocks

Previously all code blocks were in a single large index file. This was becoming unwieldy and difficult to manage/edit. Extract each large block into it’s own file for readability and maintainability.

* Updates to avoid tests depending on erroneous _id prop

Previously the `_id` prop was included purely to make testing easier (more resilient). On reflection introducing code just for tests was a bad idea. Now depends on string comparison which is an acceptable compromise.

Resolves WordPress#13899 (comment)

* Removes usage of unused attribute introduced as experiment

This attribute was introduced as part of experimenting in WordPress@e759315 and never removed. It’s unused and has been removed.

* Updates example to run without erroring due to missing var

Resolves WordPress#13899 (comment)

* Fixes comment to match coding style guidelines

Resolves WordPress#13899 (comment)

* Fixes to avoid mutation in render method

Previously in order to reset the Parent’s alignment when one of the child Columns set it’s own alignment, the render method of the Parent Columns mutated the attributes causing a re-render. To avoid this we now reset the Parent from the Child Column whose alignment changed.

Addresses WordPress#13899 (comment)

* Fixes single Column Block breadcrumb to RHS of boundary

Resolves point raised in WordPress#13899 (comment)

* Restores pass through click behaviour on single Column Block

As discussed [here](WordPress#13899 (comment)) selection of parent/child Blocks is currently being worked on elsewhere. Therefore to get the vertical-alignment shipped, this commit reverts the change that made Columns individually selectable. Now reverts to original behaviour of only being selectable via the Block Inspector (or keyboard). This assumes that future PRs will land better Parent/Child selection behaviour. Also removes unwanted space within the Columns themselves.

* Updates to tidy CSS selectors for improved readability

* Updates to move VAlign Toolbar into Block Editor package

Previously the editor package container the toolbar. Now the new Block Editor should be the new home for the toolbar

* Automated README update

* Updates changlogs to reflect release status
// These margins make sure that nested blocks stack/overlay with the parent block chrome
// This is sort of an experiment at making sure the editor looks as much like the end result as possible
// Potentially the rules here can apply to all nested blocks and enable stacking, in which case it should be moved elsewhere
// When using CSS grid, margins do not collapse on the container.
.wp-block-columns .block-editor-block-list__layout {

This comment has been minimized.

Copy link
@aduth

aduth Apr 15, 2019

Member

Why was this changed? It undoes intentional changes from #14420.

Noting that confusion around these classnames was anticipated in #14112 (comment) . As such, it might warrant more exploration about how we can eliminate the wrong names (.editor-) from code while retaining them in the built output for compatibility.

const ColumnEdit = ( { attributes, updateAlignment } ) => {
const { verticalAlignment } = attributes;

const classes = classnames( 'block-core-columns', {

This comment has been minimized.

Copy link
@aduth

aduth May 7, 2019

Member

Was this intentionally named as the plural "columns" ? This is for the single "column" block, a separate block from the plural "columns" block. Should it not then be named 'block-core-column' instead?

@aduth aduth referenced this pull request May 21, 2019

Open

Enables Block Grouping/UnGrouping using core/group #14908

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.