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

Further improve columns block. #11659

Merged
merged 3 commits into from Nov 9, 2018

Conversation

Projects
None yet
6 participants
@jasmussen
Contributor

jasmussen commented 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.

Screenshots:

screenshot 2018-11-09 at 11 00 48

GIF:

columns

@jasmussen jasmussen self-assigned this Nov 9, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Nov 9, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

@youknowriad I would appreciate thoughts on whether this should be part of the 4.3 milestone or not. The only "controversial" change in this PR, is the changing of the appender from input to textarea. What do you think?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 9, 2018

The only "controversial" change in this PR, is the changing of the appender from input to textarea. What do you think?

First impression, I think it's fine because we don't really type in it.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

Cool. I'd really like this in 4.3 as it complements the columns PR well. Going to restart the test again, because they pass locally.

@jasmussen jasmussen force-pushed the update/columns-initial-state-polish branch from 0aea826 to 8498062 Nov 9, 2018

@jasmussen jasmussen added this to the 4.3 milestone Nov 9, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

Adding to 4.3 milestone because it also ensures the placeholder text fits on mobile (it's longer as of #11653).

@aduth aduth force-pushed the update/columns-initial-state-polish branch from 8498062 to 7e01119 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.

@aduth aduth force-pushed the update/columns-initial-state-polish branch from 7e01119 to abf541d Nov 9, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 9, 2018

Rebased to resolve conflict.

The change to textarea makes sense for the wrapping, but it also seems to cause this:

image

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

Hmm, good catch. Taking a look.

@aduth

This comment has been minimized.

Member

aduth commented Nov 9, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

I'm just about to push a fix which simply uses overflow: hidden, that seems to fix it. Any objections to using that?

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Nov 9, 2018

@jasmussen You might also try a resize: none in there to prevent the textarea from resizing at all.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

Hmm, yes, resize: none definitely needs to be there, and "overflow: hidden" is only a partial fix. Looking more into it.

Fix by using TextareaAutosize.
Props @aduth.

Also adds resize: none;, props @chrisvanpatten.
@aduth

This comment has been minimized.

Member

aduth commented Nov 9, 2018

Seems to me overflow: hidden; would just crop the wrapped bit.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

Correct, both of you, and both of your suggestions make for the correct fix:

screenshot 2018-11-09 at 15 28 50

That's a columns block inside a columns block. And it's not even as horrible as you'd think that would be. (Not that I'd recommend anyone ever do this)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

Not sure why the tests are suddenly failing. Tried restarting once. Now running them again locally. They're a bit slow on my old "has a good keyboard still" Macbook.

@youknowriad

LGTM 👍

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 9, 2018

Tada! Thanks Riad for the unit test fixes. Merging when the checks pass.

@jasmussen jasmussen merged commit 5da5962 into master Nov 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the update/columns-initial-state-polish branch Nov 9, 2018

@ZebulanStanphill

This comment has been minimized.

Contributor

ZebulanStanphill commented Nov 9, 2018

@jasmussen

That's a columns block inside a columns block. And it's not even as horrible as you'd think that would be. (Not that I'd recommend anyone ever do this)

Just so you know, nesting columns like this actually comes up surprisingly often in page building. At some point in phase 2 you should be able to do stuff like this without running into any weirdness.

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Further improve columns block. (WordPress#11659)
* Further improve columns block.

This PR is a followup to thoughts in WordPress#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 WordPress#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