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

Refine selected block borders #1802

Closed
iamthomasbishop opened this issue Jan 22, 2020 · 11 comments · Fixed by #1859
Closed

Refine selected block borders #1802

iamthomasbishop opened this issue Jan 22, 2020 · 11 comments · Fixed by #1859
Labels
Proposal [Type] Enhancement Improves a current area of the editor

Comments

@iamthomasbishop
Copy link
Contributor

While working on InnerBlocks, we decided to try a new style for borders to highlight the difference between nested and non-nested blocks. I must say, after playing w/ blocks that use this method, it feels right for all blocks, so I'm proposing that we extend to use this across the board.

Essentially what we did was move the left/right borders in towards the center so that there were slight optical margins on the left/right sides of the block. The two approaches look like this in practice (both are in use at the moment):

image

Note that all blocks (other than special cases w/ columns, etc) span 100% of the content container, but we used a mix of margin and padding to define the border so that there is an optical inset. So instead of using 12px/16px as the padding on all blocks (and no margin), we use something like 12px/12px, and then there is a 1px border and 3px margins on left/right of the block (don't quote me on these exact values because I can't recall what we're using in the code). This might help to illustrate the difference:

image

There are a few wins we get by doing this:

  • Above all, it's much clearer which block is selected
  • 12/12 padding inside blocks feels more balanced
  • Right now, we have a couple of different styles for (parent) block borders depending on device size — this would consolidate into a single style
  • As we start getting into more complex layout and nesting, this should make the UI feel more natural, because elements won't feel as much like they're "bumping" against each other

// cc @lukewalczak @jbinda @pinarol @hypest — any thoughts?

@iamthomasbishop iamthomasbishop added [Type] Enhancement Improves a current area of the editor Proposal labels Jan 22, 2020
@jbinda
Copy link
Contributor

jbinda commented Jan 23, 2020

So instead of using 12px/16px as the padding on all blocks (and no margin), we use something like 12px/12px, and then there is a 1px border and 3px margins on left/right of the block (don't quote me on these exact values because I can't recall what we're using in the code).

That's the correct values and in the code it looks almost the same. In the _variables.scss file there is a definition of border connected values (see below):

Base on that each block receive proper class with already calculated value of margin. The cool thing about that are:

  1. Code simplicity and readability
  2. It's easy to play with those value and see how the bordering behaviour changes

If we use the same for all blocks we can be able e.g. to adjust margins across the app with very small effort (only by changing few variables mentioned above)

I must say, after playing w/ blocks that use this method, it feels right for all blocks, so I'm proposing that we extend to use this across the board.

I remember during work on navigation down border style I ask we should apply the same style for all blocks. My opinion about that doesn't change and I think it's more consistent that all blocks has the same selected style. So I agree with your proposal.

I also wonder if working on that isn't a good moment to go back to discussion about the logic of dimming blocks that are not selected , what do you think ?

@iamthomasbishop
Copy link
Contributor Author

Thanks for the info on the current state of the code. It should make these changes pretty straightforward from the sounds of it!

I also wonder if working on that isn't a good moment to go back to discussion about the logic of dimming blocks that are not selected , what do you think ?

I think it’s worth considering, although dimming is a much more useful indicator for nested blocks because they get more complex than non-nested ones. I’m open to suggestions for dimming/highlighting logic!

@jbinda
Copy link
Contributor

jbinda commented Jan 31, 2020

Thanks for the info on the current state of the code. It should make these changes pretty straightforward from the sounds of it!

Yeah, applying full border to other components shouldn't be complicated

Plan:
What if we apply below rules:

  1. Show Floating Toolbar for all blocks
  2. Dim all non-selected block
  3. Implement indicator that show where the selected Group starts / ends no matter how deep with selection in structure we go
  4. Apply breadcrumb animation described here

Motivation:
One of the feature that I missed during play with the app is to easy way to deselect current selection. In InnerBlock you can do that with "Go Up" button on FloatingToolbar. Showing FloatingToolbar to other block brings two things:

  1. Allow to deselect
  2. Show the current hierarchy

Deselect feature also allow user to "preview" how the designed post is look like without any additional indicators.

Dimming all unselected block bring some consistency according to how it works on InnerBlock.
Personally I would also go with some nice-looking animation during selection change as we discuss some time ago.

WDYT ?

@pinarol
Copy link
Contributor

pinarol commented Jan 31, 2020

I like the idea of equalizing the paddings/margins between nested blocks and others.

Right now, we have a couple of different styles for (parent) block borders depending on device size — this would consolidate into a single style

This would bring simplicity into the UX as well as into the code.

@iamthomasbishop
Copy link
Contributor Author

@jbinda responding to your notes above:

Show Floating Toolbar for all blocks

I think it's worth a try, especially if we can get to the "ideal" version of the toolbar.

Dim all non-selected block

I'm on the fence about dimming. Maybe we should leave dimming for a special "mode" (similar to web's "spotlight mode") so it is an explicit user decision and the behavior is universal based on that toggle?

Regardless of how you get to/see this spotlight mode, can I ask to clarify what the logic would be for dimming? My understanding is that we would dim everything "above" (in the nesting hierarchy) or "outside" of the currently-selected block. In other words, if a block that has InnerBlocks is selected, itself and it's children would be fully visible, and everything outside of that would be dimmed. Is that correct?

Implement indicator that show where the selected Group starts / ends no matter how deep with selection in structure we go

I'm not 100% sure what you mean here.

Apply breadcrumb animation described [here] (WordPress/gutenberg#19960 (comment))

I think that's worth a try, seems like it'd be a nice improvement.

One of the feature that I missed during play with the app is to easy way to deselect current selection.

Totally agree. My original idea was that tapping outside of the selected parent would essentially escape out of the current selection (this was with dimming — the idea was that the dimming would behave in the same way that a modal backdrop/scrim would, where tapping it dismisses the modal). This would be a nice improvement but I think it can probably be added as an iteration, not a blocker at this time.

@jbinda
Copy link
Contributor

jbinda commented Jan 31, 2020

I think it's worth a try, especially if we can get to the "ideal" version of the toolbar.

I have read that its already in progress so soon we can start to give it a shot to all blocks

In other words, if a block that has InnerBlocks is selected, itself and it's children would be fully visible, and everything outside of that would be dimmed. Is that correct?

At this moment it works as you described it.

I'm on the fence about dimming.

I don't have strong opinion on dimming in way I mentioned. Probably we can see if the current dimming logic is still fine after we apply the solid full border to all blocks.

I think that's worth a try, seems like it'd be a nice improvement.

Great !

Totally agree. My original idea was that tapping outside of the selected parent would essentially escape out of the current selection (this was with dimming — the idea was that the dimming would behave in the same way that a modal backdrop/scrim would, where tapping it dismisses the modal). This would be a nice improvement but I think it can probably be added as an iteration, not a blocker at this time.

Sure we can evaluate it later - small steps is better :)

Summarising all above I see three action items at this moment:

  1. Apply selection solid border style to all blocks
  2. Prepare POC on animate breadcrumb to clearly see selection changes in InnerBlocks
  3. Enable FloatingToolbar for all blocks (after its ideal version will be shipped)

Is that correct ?

@iamthomasbishop
Copy link
Contributor Author

  1. Apply selection solid border style to all blocks

Just to clarify, this applies to all selected blocks (not children, which get dashed borders).

  1. Enable FloatingToolbar for all blocks (after its ideal version will be shipped)

Let's try it and see how it feels 😄

@jbinda
Copy link
Contributor

jbinda commented Feb 4, 2020

Ok, will prepare working example and share results

@jbinda
Copy link
Contributor

jbinda commented Feb 4, 2020

@iamthomasbishop

Please check this out :)

@iamthomasbishop
Copy link
Contributor Author

As discussed in Slack, the borders look great, and the floating toolbar works as expected. I think we can ship this and iterate if necessary.

Also, (in a separate ticket) we might want to consider removing the up icon/indicator on top-level blocks. I know this is something we had discussed a while back, but had decided to leave it and see how it feels. After using it for a while (on various nested blocks) I think it would make sense to remove on top-level blocks.

@hypest
Copy link
Contributor

hypest commented Feb 28, 2020

I think it would make sense to remove on top-level blocks.

Just wanted to add a +1 to removing the "Up" button for top level blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal [Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants