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: Add width attribute for resizable Column blocks #15499

Merged
merged 12 commits into from May 12, 2019

Conversation

@aduth
Copy link
Member

commented May 8, 2019

This pull request seeks to implement an initial exploration of resizable column blocks. It should serve as some foundation upon which to improve the usefulness of the Columns block.

columns

In this first iteration:

  • An empty Column block displays with the "Button" appender introduced in #14241, similar to the Group block (#14943). The thinking is that this allows for a simpler gauge of columns width before adding content.
  • An individual column block can be assigned with a width attribute which overrides the default equal distribution of column widths in a Columns block. Widths of all adjacent Column blocks are updated automatically, and widths redistributed when a Column is added or removed from the Columns block.

Not yet included, but planned for future iterations:

  • A draggable resizer handle to adjust columns width. I have the bulk of the code for this complete already.
  • Quick-select "template" options for columns distribution when initially creating a Columns block.
  • Improved methods to select a Column and a Columns block to adjust settings.

Specific Behavior:

  • When changing the width of a Column, the amount of increase or decrease is respectively removed or added in equal distribution across the Column blocks after the block being changed, unless the Column block is the last in the set (in which case, the redistribution occurs in reverse).
  • When adding or removing a Column to a Columns block, the new Column is assigned a width as if there were equal distribution. For example, increasing Columns from 2 to 3 would assume a width of 33.3% for the new block (100 / 3). This width (or, in the case of removal, the width of the removed block) is removed from or added to other blocks in the Columns.
  • The default behavior of Columns remains unchanged in that width is only assigned or adjusted if a width value was assigned to a Column block in the set. This also helps ensure backwards-compatibility with previously-existing Column blocks.

Testing Instructions:

Verify that you can resize a Column block, and that in doing so, adjacent Column blocks are resized accordingly. Similarly, verify that adding or removing new Columns should redistribute widths to/from other columns.

  1. Navigate to Posts > Add New
  2. Insert a Columns block
  3. Select the Column block (ArrowUp and ArrowDown are reliable here)
  4. Adjust width value in the Inspector
  5. Observe that the second column updates its width accordingly (to a sum of 100)
  6. Select the Columns block
  7. Increase / decrease Colujmsn count
  8. Observe that the previous two columns update their widths accordingly (to a sum of 100)
  9. Repeat steps 3 through 8 with a variation of Column combinations

Remaining tasks:

  • There is occasionally some issue in columns redistribution related to precision, where the sum total of Column widths may not equal 100 (but rather, e.g. 99.98%)
@Soean

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Looks very promising.
Maybe we should add a help text about the unit. It's percent %, not pixel px.

@paulwilde

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

It would be ideal if the columns could snap into a grid of 12 as is common with most grid foundations such as Bootstrap, or at least if there's a preference to enable such functionality. Reason being this allows for a consistent grid on a website rather than having random widths on the page.

It would also allow for each column to use classes instead of inline styles which are quite troublesome when it comes to making these columns responsive, especially if the ability to set different widths on different breakpoints were to be added in the future.

renderAppender={ (
hasChildBlocks ?
undefined :
() => <InnerBlocks.ButtonBlockAppender />

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 8, 2019

Contributor

I like this change :)

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 8, 2019

Contributor

Do you why the sibling inserter is not shown when there's a single column with a single block? It feels like a separate issue but it's made more obvious with this change

This comment has been minimized.

Copy link
@aduth

aduth May 9, 2019

Author Member

Do you why the sibling inserter is not shown when there's a single column with a single block? It feels like a separate issue but it's made more obvious with this change

I think it's the same as the fact that there is no sibling inserter shown at the end of the block list, only the "default appender". The column is effectively the same as the top-level block list.

However, I don't necessary agree that the sibling inserter shouldn't be shown after the last block in a block list. I think there may have been an issue about this at some point?

updateAlignment( verticalAlignment ) {
const { clientId, setAttributes } = ownProps;
const { updateBlockAttributes } = dispatch( 'core/block-editor' );
const { getBlockRootClientId } = registry.select( 'core/block-editor' );

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 8, 2019

Contributor

Nice little perf tweak

This comment has been minimized.

Copy link
@aduth

aduth May 8, 2019

Author Member

Nice little perf tweak

Thanks for reminding: I'd meant to reference some commentary at #13899 (comment) which had led to some of this general (but related/required) refactoring.

};
} ),
withDispatch( ( dispatch, { clientId, parentColumnsBlockClientId } ) => {
withDispatch( ( dispatch, ownProps, registry ) => {

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 8, 2019

Contributor

Nit: Any reason you didn't destructure registry

This comment has been minimized.

Copy link
@aduth

aduth May 8, 2019

Author Member

Nit: Any reason you didn't destructure registry

I'm personally not a fan of destructuring in an arguments signature, since it loses context of what it is that is being destructured. To a lesser degree, I find it hard to visually distinguish between proper arguments, and properties destructured (attention to the { and }).

// Update own width.
setAttributes( { width } );

// Constrain or expand siblings to account for gain or loss of

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 8, 2019

Contributor

I wonder if the computations here could be extracted to a unit-testable pure function.

This comment has been minimized.

Copy link
@aduth

aduth May 8, 2019

Author Member

I wonder if the computations here could be extracted to a unit-testable pure function.

Yeah, I expect there's a fair bit of clean-up which could be done here 😅 Perhaps even some reusability between redistribution logic between the individual Column and its parent Columns block.

This comment has been minimized.

Copy link
@aduth

aduth May 10, 2019

Author Member

I wonder if the computations here could be extracted to a unit-testable pure function.

Still generally more complex than I'd have liked, but improved refactoring and tests in 6880cfa.

const wrapperClasses = classnames( {
[ `is-vertically-aligned-${ verticalAlignment }` ]: verticalAlignment,
} );

let style;
if ( Number.isFinite( width ) ) {
style = { flexBasis: width + '%' };

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 8, 2019

Contributor

is this kses allowed (just to make sure we check as all inline style properties are not allowed)?

This comment has been minimized.

Copy link
@aduth

aduth May 8, 2019

Author Member

is this kses allowed (just to make sure we check as all inline style properties are not allowed)?

Good catch! It is not.

I will need to filter this set, and follow-up with a Trac ticket.

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/kses.php?rev=45242#L2057

This comment has been minimized.

Copy link
@aduth

aduth May 10, 2019

Author Member

is this kses allowed (just to make sure we check as all inline style properties are not allowed)?

Added in 5bb641c

return;
}

// Redistribute available width for existing inner blocks.

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 8, 2019

Contributor

Same, I wonder if this could be unit testable

@youknowriad
Copy link
Contributor

left a comment

Overall, the PR seems great to me. It would open a lot more possibilities for follow-up improvements (drag and drop, setup state)

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@Soean you're commenting from the future mate :) (Github bug)

@mapk

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

I just tested today and immediately loved the new Button appenders in the columns! Fantastic indicators and visual reference for the columns.

I've got a few notes for some improvements before we merge this.

  1. The column width slider defaults with no number in it. But it's set at 50%. Can we add the number there by default?
  2. It would be great to add a % sign, or maybe just change the label to say Percentage width for the column width slider so that it's obvious we're setting percentage widths.
  3. Can we also add a Reset button to return the columns back to the default setting?

Something like this:

allgood

@mapk

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Or maybe the reset button can be similar to the Color Settings for the paragraph block and say, clear instead? This way we follow a pattern.

Screen Shot 2019-05-08 at 4 00 31 PM

@draganescu

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Hi @aduth I've tested and found that the width in certain cases (kinda extreme) make the columns flicker:

collumns-flicker

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@draganescu I had seen that on an occasion as well. It's odd, because the only effective difference in markup generated is the application of a flex-basis style, which shouldn't seem like it ought to have this effect. I'd wondered if it was perhaps a browser bug. If you are able to reproduce it, could you try viewing it in another browser?

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Thanks for the feedback, all. I'll plan to apply it today.

@aduth aduth referenced this pull request May 10, 2019

Merged

Remove inserter inside multi-selection #3246

3 of 3 tasks complete
@aduth

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I spent the better part of the day trying to work with the end-to-end tests to improve their stability. I think they've always been quite unstable surrounding these specific behaviors, but the specific introduction of the button block appender made things even more difficult.

I'm not particularly happy with the inevitable solution I came up with for the tests to pass in a840fd9, but I think there are some larger problems with the tests that need a more focused effort, notably:

  • Delays in Fill unmounting when a new set of Fills occupies a Slot (when switching from one block to another)
  • BlockList rendering as async both for:
    • Inner blocks of a block
    • A block which receives focus but has not yet become selected (i.e. WritingFlow arrow navigation, splitting, merging)

Since these impact exclusively the tests and the behavior of the columns is otherwise agreed upon, I'd personally suggest we merge this in its current form and iterate on test improvement separately.

@youknowriad
Copy link
Contributor

left a comment

Makes sense to me. Let's work on these tests separately.

@@ -26,11 +29,9 @@

@include break-small() {

// Beyond mobile, allow 2 columns.
flex-basis: calc(50% - #{$grid-size-large});

This comment has been minimized.

Copy link
@aduth

aduth May 10, 2019

Author Member

I'm assuming this wasn't strictly necessary, also by the fact that it (wrongly) assumes a Columns block would only ever have 2 columns. In some testing, the default width distribution of a columns without widths assigned (flex-basis: auto;) appears to be equal distribution.

This comment has been minimized.

Copy link
@aduth

aduth May 10, 2019

Author Member

I'm assuming this wasn't strictly necessary, also by the fact that it (wrongly) assumes a Columns block would only ever have 2 columns. In some testing, the default width distribution of a columns without widths assigned (flex-basis: auto;) appears to be equal distribution.

Actually, in some final testing, it does appear to make a difference. I'm not sure if it's intended that mid-range viewports show as two-column, but that's how it behaves in master, and I'd rather leave it as it was.

image

(Note: This applies only to Columns blocks where widths are not explicitly assigned)

This comment has been minimized.

Copy link
@m-e-h

m-e-h May 10, 2019

Member

Your assumptions sound correct here @aduth
The original design had 2 columns always showing at the medium breakpoint. You may not have noticed it cause It didn't actually work for the first couple versions of the editor. Got fixed here #12199

I don't think that two column breakpoint would be missed too much if removing it would help what you're doing here.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Hi @aduth I've tested and found that the width in certain cases (kinda extreme) make the columns flicker:

This seems to happen when columns contain images. In some cross-browser testing, I believe it's an issue specifically affecting Chrome. It's not a great experience surely, but based on my previous comment on how limited the DOM changes to a Column block is with this branch, I strongly suspect this is a browser rendering bug largely out of our control.

@paaljoachim

This comment has been minimized.

Copy link

commented May 11, 2019

This feels like one of multiple very useful steps/iterations that will affect width, height, padding, margin, drag, responsive, grid and more of multiple blocks (perhaps all blocks). It would be great with a writeup of this step and thoughts on future steps. So that others could also share feedback. Thanks!

@aduth aduth merged commit 5494bc5 into master May 12, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the update/columns-width branch May 12, 2019

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 13, 2019

@mapk

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

🎉 Excellent work!

Will there be another PR soon that works toward the dragging resizing option that Joen mentioned above?

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Will there be another PR soon that works toward the dragging resizing option that Joen mentioned above?

I'll create some follow-up issues today.

@StaggerLeee

This comment has been minimized.

Copy link

commented May 15, 2019

There are already Bootstrap Gutenberg blocks (for columns and much more) that you will never come close to. And you lose your time to make everything from the scratch, when there is almost perfect Bootstrap library to integrate.
I do not understand it.

Does Matt Mullenweg pay you per code lines ?

Ah OK, now continue to adapt them to responsive views.

@SchneiderSam

This comment has been minimized.

Copy link

commented May 15, 2019

If I enter 33 in one of my columns, the entire column layout will be destroyed. Since the page is online, I can't show it live. But somewhere there are still problems.

Login: rb
PW: buvexayu
URL: http://b320kx.myraidbox.de/

Normally it should look like this:
Screenshot_3

but in the live site it looks like this:
Screenshot_4

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@StaggerLeee That's an awfully cynical stance to hold, and I think you could express your point without resorting to sarcasm or abuse. It's fine for a plugin and core functionality to coexist in the same space. The WordPress philosophy advocates design for the majority, simplicity, and out-of-the-box usefulness. A plugin can add specialized functionality, and simply absorbing one plugin's implementation isn't always the best option for the baseline offering; though it can certainly inform it, such as is already taking place in follow-up tasks (e.g. #15662 "Prior art"). If you have specific feedback, I'd encourage you to provide it (constructively) in one of the existing issues, a new issue, or to participate in one of the weekly meetings.

@SchneiderSam Could you create a new issue for this? It may be a specific incompatibility with your theme, but a dedicated issue would be the more appropriate place for debugging.

@nerrad

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@StaggerLeee please take some time to read the Code of Conduct for this repository. Your comments could be seen as breaching some of the principles of that code. Constructive feedback is always welcome here but repeated abuse of the code of conduct will not be tolerated.

@karmatosed

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@StaggerLeee whilst I understand you may have a strong opinion here, @nerrad is correct there is a code of conduct that applies to this repo. I would ask you to please be mindful of this in the future. Every person here is doing their best and individual humans that deserve respectful interactions. Let's focus on all creating the best product we can together.

Thank you @nerrad for your response on this issue 🙏

@StaggerLeee

This comment has been minimized.

Copy link

commented May 15, 2019

What a fuss, just for mentioning the Boss. My apologies.

@karmatosed

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Whilst again I understand your feelings here, the code of conduct is taken seriously within this repo and WordPress itself. Let's draw a line for now under this and move on with that clear understanding for future interactions.

@m-e-h

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@StaggerLeee have you checked out the Joomla CMS? Pretty sure it still includes the bootstrap framework in it's core. Of course there are challenges to such an approach but it sounds like it may be closer to what you're looking for.

I don't get paid anything for my code here 🤷‍♂

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.