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

Try: Improve columns block #11620

Merged
merged 2 commits into from
Nov 9, 2018
Merged

Try: Improve columns block #11620

merged 2 commits into from
Nov 9, 2018

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Nov 8, 2018

This PR aims to improve the column block usability, as well as fix a few issues:

  1. It makes the individual columns unclickable by using pointer events. Since these are not actionable yet, being able to select them is not really helpful.

  2. It improves the UI around reusable blocks, by fixing an issue where even though contents should be disabled when in a reusable block, it was still selectable if there were nested blocks.

  3. It further improves the reusable block UI by fixing a regression where the reusable block editing bar did not push down content sufficiently.

  4. It refactors how margins in the columns block work, to be simpler and more predictable, and makes it behave similar to every other block in that regard.

JIFs

How reusable blocks work now:

reusable blocks

Previously there wasn't enough margin below the gray bar, and the "Save" button was hard to click.

Columns:

columns

Previously you could select individual columns, but these columns were not actionable. This PR makes them unclickable.

As part of phase 2, I expect we will continue to iterate the UI for selecting blocks, notably in nested contexts, and as part of that we can consider making the individual columns selectable again. But for now, this seems like a good solution.

Please share your thoughts.

@jasmussen jasmussen added the [Status] In Progress Tracking issues with work in progress label Nov 8, 2018
@jasmussen jasmussen self-assigned this Nov 8, 2018
@jasmussen jasmussen force-pushed the try/improve-columns branch 2 times, most recently from 5fbba12 to cf82200 Compare November 8, 2018 13:49
@jasmussen jasmussen removed the [Status] In Progress Tracking issues with work in progress label Nov 8, 2018
@jasmussen jasmussen requested review from aduth, mtias and a team November 8, 2018 13:52
@jasmussen jasmussen added [Type] Enhancement A suggestion for improvement. [Type] Bug An existing feature does not function as intended labels Nov 8, 2018
@jasmussen jasmussen added this to the 4.4 milestone Nov 8, 2018
This PR aims to improve the column block usability, as well as fix a few issues:

1. It makes the individual columns unclickable by using pointer events. Since these are not actionable yet, being able to select them is not really helpful.

2. It improves the UI around reusable blocks, by fixing an issue where even though contents should be disabled when in a reusable block, it was still selectable if there were nested blocks.

3. It further improves the reusable block UI by fixing a regression where the reusable block editing bar did not push down content sufficiently.

4. It refactors how margins in the columns block work, to be simpler and more predictable, and makes it behave similar to every other block in that regard.
@youknowriad youknowriad modified the milestones: 4.4, 4.3 Nov 8, 2018
@mtias mtias added the [Block] Columns Affects the Columns Block label Nov 8, 2018
@notnownikki
Copy link
Member

It makes the individual columns unclickable by using pointer events. Since these are not actionable yet, being able to select them is not really helpful.

This is a solid improvement, far fewer blue boxes appearing.

It improves the UI around reusable blocks, by fixing an issue where even though contents should be disabled when in a reusable block, it was still selectable if there were nested blocks.

It further improves the reusable block UI by fixing a regression where the reusable block editing bar did not push down content sufficiently.

Great! Feels much better, again!

It refactors how margins in the columns block work, to be simpler and more predictable, and makes it behave similar to every other block in that regard.

Yes, it felt weird before to have the column selectable, the inner paragraph selectable, and the columns themselves selectable, and it was a matter of pixels movement to go from one to the other. Although I still need to use the block navigation to select the columns themselves, this PR makes things feel a lot better.

I'll hold back from approving for now just because it's a change to the UX that I think should have a couple more eyes on, but it's a 👍 from me!

@chrisvanpatten
Copy link
Contributor

I wish preventing individual columns from being clicked was handled via a supports flag or something like that (so it could also prevent a column from being selected via the block outline panel thingy, and easily be available to other blocks) but this feels like a decent stop-gap.

Do we still have a ticket open for that "pass-through" concept?

@jasmussen
Copy link
Contributor Author

Do we still have a ticket open for that "pass-through" concept?

I believe it's this one: #7694

@jasmussen
Copy link
Contributor Author

Pushed a fix for the empty columns, that further refactors.

There's one thing left that I'd like to fix, and it's the "initial state". Hard to explain, so here's a GIF:

weird hover

So, the pointer-event stuff I use as a half-way method to disable the column blocks, singular, doesn't take when the default appender is there. @aduth do you have bandwidth for a sanity check on this one?

I think we can ship this as an iterative improvement even with that still in place, but it'd be a nicer experience if we could disable the column block even when it only contains the appender.

pointer-events: none;
}

:not(.components-disabled) > .wp-block-columns > .editor-inner-blocks > .editor-block-list__layout > [data-type="core/column"] > .editor-block-list__block-edit > .editor-inner-blocks {
Copy link
Contributor

Choose a reason for hiding this comment

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

What a selector :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Have to be careful with the pointer events! I look forward to when we have a more official passthrough method.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

WFM 👍

@jasmussen
Copy link
Contributor Author

jasmussen commented Nov 9, 2018

Thank you. I will let you merge if you feel it's good to go — I certainly think it is, though I would like to then address #11620 (comment) in a separate PR.

Edit: Upon second thought, this adds much needed bugfixes and works well. I can enhance the initial hover issue separately. Merging.

@jasmussen jasmussen merged commit 8e396e7 into master Nov 9, 2018
@jasmussen jasmussen deleted the try/improve-columns branch November 9, 2018 08:41
aduth pushed a commit that referenced this pull request Nov 9, 2018
This PR is a followup to thoughts in #11620 (comment).

It improves the columns block further by doing two things:

1. It hides the hover effect on the individual _column_ block in the initial state.

2. It changes the non-block "fake" appender to be a textarea instead of an input.

To elaborate on 1, in #11620 we used pointer-events to disable the individual column blocks as selectable using the mouse. This is because those columns are not actionable at the moment, and in the current implementation of the column, being able to hover and select them only causes issues with selecting the parent columns block, which _is_ actionable.

However pointer events was not enough for the _empty_ state of a columns block. In this state, the block features no inner blocks, except the individual column blocks, because no content has yet been inserted yet. In this state, you see the "appender", which is not actually a block even though it looks exactly the same as an empty paragraph. Because this is not technically a block, the deepest nested child is a _column_, which is then allowed to be hovered.

In 1 I add a workaround to that, to simply visually hide the hover effect. I expect this to be refactored in a future columns update, but for the time being it makes it far more usable.

To elaborate on 2, there has been a long time issue with the appender (again, not the block, the fake version that we use before an empty paragraph is inserted) wrapping text in translations. This is because text _cannot_ wrap in an `input` field, which the appender is. By changing this to a `textarea`, the text can wrap.

This might affect themes which specify the input element for increased specificity in their editor styles, but overall it still seems like a worthwhile change to make, since it's the only way we can allow the writing prompt to wrap its text.
jasmussen added a commit that referenced this pull request Nov 9, 2018
* Further improve columns block.

This PR is a followup to thoughts in #11620 (comment).

It improves the columns block further by doing two things:

1. It hides the hover effect on the individual _column_ block in the initial state.

2. It changes the non-block "fake" appender to be a textarea instead of an input.

To elaborate on 1, in #11620 we used pointer-events to disable the individual column blocks as selectable using the mouse. This is because those columns are not actionable at the moment, and in the current implementation of the column, being able to hover and select them only causes issues with selecting the parent columns block, which _is_ actionable.

However pointer events was not enough for the _empty_ state of a columns block. In this state, the block features no inner blocks, except the individual column blocks, because no content has yet been inserted yet. In this state, you see the "appender", which is not actually a block even though it looks exactly the same as an empty paragraph. Because this is not technically a block, the deepest nested child is a _column_, which is then allowed to be hovered.

In 1 I add a workaround to that, to simply visually hide the hover effect. I expect this to be refactored in a future columns update, but for the time being it makes it far more usable.

To elaborate on 2, there has been a long time issue with the appender (again, not the block, the fake version that we use before an empty paragraph is inserted) wrapping text in translations. This is because text _cannot_ wrap in an `input` field, which the appender is. By changing this to a `textarea`, the text can wrap.

This might affect themes which specify the input element for increased specificity in their editor styles, but overall it still seems like a worthwhile change to make, since it's the only way we can allow the writing prompt to wrap its text.

* Fix by using TextareaAutosize.

Props @aduth.

Also adds resize: none;, props @chrisvanpatten.

* Fix unit tests
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 [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants