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

Improving interaction between parent and child blocks #14148

Closed
wants to merge 2 commits into from

Conversation

mapk
Copy link
Contributor

@mapk mapk commented Feb 27, 2019

In response to the discussion in #9628, and relation to this comment by @gziolo, I thought I'd try this out.

I added 10px padding to .editor-inner-blocks. to help improve interaction between the parent and child inner blocks. The selector exists already for all blocks that have inner blocks. I just defined the attributes of that selector.

padding2

How has this been tested?

I've tested locally.

Types of changes

Pure CSS.

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.

@mapk mapk added the Needs Design Feedback Needs general design feedback. label Feb 27, 2019
@mapk mapk self-assigned this Feb 27, 2019
@mapk mapk requested review from gziolo, kjellr and jasmussen February 27, 2019 17:37
@mapk mapk requested a review from chrisvanpatten as a code owner February 27, 2019 17:37
@mapk
Copy link
Contributor Author

mapk commented Feb 27, 2019

The downside here is that it makes the inner blocks more narrow.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Thanks, @mapk! This is a good start, and it does help make these blocks a bit more navigable. I think the key thing that's missing is that we should only add an additional border here on select. Currently it happens all the time.

Unfortunately, our is-selected class only gets applied to the innermost child right now. And since CSS doesn't allow for working back up the document tree, there's no way to assign styles to the parents of those blocks. If we gave parent blocks a child-is-selected/has-child-selected class or something, we could get this working pretty seamlessly.

packages/editor/src/components/inner-blocks/style.scss Outdated Show resolved Hide resolved
@mapk
Copy link
Contributor Author

mapk commented Feb 27, 2019

I think the key thing that's missing is that we should only add an additional border here on select. Currently it happens all the time.

I was thinking about this too, but I realized that trying to select the parent block right out of the gate is difficult. So having the extra padding on at all times seemed worked better.

parent

I'm not sure what to do here. Is this a good solution? Or would showing the padding only when selected be better?

@kjellr
Copy link
Contributor

kjellr commented Feb 27, 2019

Is this a good solution? Or would showing the padding only when selected be better?

I lean towards only adding the padding on select, so that we can still preserve a deselected state that looks close to what you'd see on the frontend.

I could totally be convinced otherwise though — this does definitely help improve selection in general, and that's definitely needed.

@chrisvanpatten
Copy link
Contributor

If we had gave parent blocks a child-is-selected/has-child-selected class or something, we could get this working pretty seamlessly.

I would LOVE this. I don't have the understanding of the BlockList to make this happen, but would majorly benefit from this.

@kjellr
Copy link
Contributor

kjellr commented Feb 27, 2019

Also worth noting: #13899 does add a bit extra of top/bottom spacing to columns in order to ensure users can select the interior column block. It does not add space to the left/right of interior blocks though:

columns

I was just switching between that PR and this one, and it does seem that the extra left/right padding in this PR makes these much easier to select.

@mapk
Copy link
Contributor Author

mapk commented Feb 27, 2019

I could totally be convinced otherwise though

I'm going to attempt to. 😉 Just kidding. I totally get the feeling that an unselected block should represent the final product as much as possible.

After looking at the $block-padding variable (14px), it feels really wide. Do you think it goes against standards to run a calc( $block-padding / 2 ) on it instead? This would drop it to 7px which might also work pretty well in the unselected state too.

@jasmussen
Copy link
Contributor

Thanks for taking a stab.

Editor:

screenshot 2019-02-28 at 10 04 31

Frontend:

screenshot 2019-02-28 at 10 04 39

This definitely makes selecting the columns block easier, and if we revert changes to disallow you to select the column blocks (singular), I'm sure those would be easier to select too.

But to the points already made on this thread (agree with both Kjell and Chris), I think we need to tread slowly and intently here. With the padding always in place regardless of selection state, there's now a rather big disconnect between frontend and editor, and I'm not sure the payoff is worth it.

Keep in mind that things like the Quote and possibly many other blocks such as List and Table, may gain support for inner blocks in the future. So we need something that scales across the board, and not just assume that only complex blocks are going to need improvements in selecting parent blocks.

Here's how I imagine things might work:

  • Unselected columns looks the same in the editor and frontend.
  • You click inside the columns block, you're likely to select a Paragraph inside, in doing so.
  • As soon as you select this Paragraph block, a has-child-selected class is applied to the column block (singular), because that one is selectable too. With that class in place, the column block now shows a permanent border, so long as a child of it is selected, and its padding is increased drastically to make it easier to select (and the padding animates in with delightful elasticity to better communicate what's going on).
  • The added padding to the column block makes it easier to hover and select.
  • When you select the column block, the has-child-selected class is now applied to the columns block, and the padding is transferred there instead.

Sure, maybe that means you can't trivially click the Column block from the start to select that. But this feels like a separate problem, one that might best be solved using the "click-through" method as described in #9628. This is a pattern you might recognize from Illustrator, Sketch, Figma and many other apps, where you have to doubleclick a group to drill down and select a nested item inside that group. Whether we did that through doubleclick or singleclick matters less.

But the increased padding on the parent when a child is selected seems like a solid path forward to vastly improve the current difficulty in selecting the parent at all.

CC: @getdave because you're working to add features to the column (singular), and CC: @youknowriad becuase I recall you building a has-child-selected class already one time in the past, and we might want to resurrect it.

@kjellr
Copy link
Contributor

kjellr commented Feb 28, 2019

After looking at the $block-padding variable (14px), it feels really wide. Do you think it goes against standards to run a calc( $block-padding / 2 ) on it instead? This would drop it to 7px which might also work pretty well in the unselected state too.

If the block padding there is too wide, I'd recommend switching to $grid-size (8px) instead.

@mapk
Copy link
Contributor Author

mapk commented Mar 1, 2019

Closing this based on discussion that this may not be the best route. More prototypes were explored that feel like a better direction. #9628 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants