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

Block Library: Move Column resizing to Columns #16077

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 10, 2019

Closes #15660

This pull request seeks to move management of the individual Column widths to be managed at the Columns wrapper. The goal here, in combination with #16024 and toward #7694, is to eliminate the "Column" block as a necessary stop for user interaction.

resize

Accessibility Notes:

The current implementation renders a fieldset for each column. At the moment, there is only the width field within these groups. However, as part of #7694, it will be necessary to incorporate column vertical alignment into the Columns sidebar as well. With each column possibly having multiple fields to manage in the block inspector, the fieldset grouping felt appropriate. Guidance here would be appreciated.

Implementation Notes:

The implementation of the widths assignment is ported directly from the Column block, with minor adaptation to receive clientId as an argument of the function.

This nicely allows for a re-consolidation of the width sizing utility functions (previously, the "Column" block implementation penetrated the block abstraction into the implementation of the "Columns" block).

Testing Instructions:

Repeat testing instructions from #15499, managing column width from the Columns block instead of the Column block.

@aduth aduth added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility [Block] Columns Affects the Columns Block labels Jun 10, 2019
@gziolo
Copy link
Member

gziolo commented Jun 12, 2019

I guess that logic has changed for Column block causing some e2e test failures related to Block Navigation feature:

https://travis-ci.com/WordPress/gutenberg/jobs/206834450#L874

AIL packages/e2e-tests/specs/block-hierarchy-navigation.test.js (11.585s)
  ● Navigating the block hierarchy › should navigate using the block hierarchy dropdown menu
    TypeError: Cannot read property 'click' of undefined
      47 | 			"//button[contains(@class,'block-editor-block-navigation__item') and contains(text(), 'Column')]"
      48 | 		) )[ 3 ];
    > 49 | 		await lastColumnsBlockMenuItem.click();
         | 		                               ^
      50 | 
      51 | 		// Insert text in the last column block.
      52 | 		await pressKeyTimes( 'Tab', 5 ); // Tab to inserter.
      at click (specs/block-hierarchy-navigation.test.js:49:34)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:45:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:271:22)
      at Generator.prototype.(anonymous function) [as next] (../../node_modules/regenerator-runtime/runtime.js:97:21)
      at asyncGeneratorStep (specs/block-hierarchy-navigation.test.js:11:103)
      at _next (specs/block-hierarchy-navigation.test.js:13:194)
  ● Navigating the block hierarchy › should navigate block hierarchy using only the keyboard
    Execution context was destroyed, most likely because of a navigation.
      at rewriteError (../../node_modules/puppeteer/lib/ExecutionContext.js:135:15)

};

forEach( nextColumnWidths, ( nextColumnWidth, columnClientId ) => {
updateBlockAttributes( columnClientId, { width: nextColumnWidth } );
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to make updateBlockAttributes more flexible or introduce some other action for batch updates like this one. In the case when there are 6 columns, it might trigger re-render 6 times for every atomic block update. Regardless of performance considerations, I anticipate that with nested blocks starting to be more and more popular we need to start thinking about batching actions to make APIs simpler to use.

Copy link
Member Author

@aduth aduth Jun 25, 2019

Choose a reason for hiding this comment

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

Another issue here might be that we add a number of Undo levels for each change (already affects master).

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like we need to start working on undo transactions soon. This has came up in other PRs recently.

Copy link
Member Author

Choose a reason for hiding this comment

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

It feels like we need to start working on undo transactions soon. This has came up in other PRs recently.

Yeah, it's similar to #8119, but I wonder if there might be some simple near-term solution:

wp.data.dispatch( 'core/editor' ).transact( () => {
	wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes( /* ... */ );
	wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes( /* ... */ );
} );
// `'START_TRANSACTION'`
// `'UPDATE_BLOCK_ATTRIBUTES'`
// `'UPDATE_BLOCK_ATTRIBUTES'`
// `'END_TRANSACTION'`

(or just startTransaction + endTransaction / toggleTransaction actions)

Probably worth a separate issue for tracking and discussion.

@paaljoachim
Copy link
Contributor

paaljoachim commented Feb 11, 2020

Looking at the process today and how it can be improved.

User selects a block and chooses a variation to start with.
Screen Shot 2020-02-11 at 18 58 32

Lets say that I choose 2 columns.
Screen Shot 2020-02-12 at 00 30 16


With the new suggestion Andrew added above (as I see it).
The focus needs to stay on the parent block after the user has selected a variation or clicked skip. So that we see the amount of Columns and the Column Settings in the sidebar that the user can manually adjust.

Screen Shot 2020-02-11 at 19 02 13

Thinking out loud...
Here is a way to also use direct manipulation for the column width (this is in addition to the sidebar controls). As the parent column is selected one could perhaps also see a percentage box below each column. Some food for thought....
Column-direct-percentage-Gutenberg

Bottom line is making it easier on the user to select the width. Sidebar controls is a very good idea and I also think we need direct manipulation of each column when the parent column block is selected. Clicking into each column the percentage block could go away.
Keeping specific controls for the parent column.

Slack Design channel discussion: https://wordpress.slack.com/archives/C02S78ZAL/p1581442639343500

@aduth
Copy link
Member Author

aduth commented Jun 5, 2020

It's fallen quite out of date. I know there's a lot of interest in improving this experience, and some people (e.g. @ItsJonQ) actively looking at this block. At least in this particular proposal, I think it really depends on some separate exploration of nested block selection. The implementation also conflicts a bit with #19515, which removed automatic resizing of sibling column blocks.

I'll close this, but the tracking issue remains open at #15660 for further discussion and exploration.

@aduth aduth closed this Jun 5, 2020
@aduth aduth deleted the move/column-sizer-to-columns branch June 5, 2020 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block [Feature] Blocks Overall functionality of blocks Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Library: Column: Move resizing controls to Columns block
4 participants