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: Show horizontal movers for Social Links. #17627

Open
wants to merge 1 commit into
base: master
from

Conversation

@jasmussen
Copy link
Contributor

commented Sep 27, 2019

This is more of a proof of concept and experiment, than it is final PR. So do not merge as is.

We might find that we like this, in which case it might need a little polish, and then we can ship it. But for now this PR has been created to help further discussion in #16637.

This PR does the following:

  • Starts with Social Links, because that is the first block to "absorb child block UI", and therefore a good test-bed for this.
  • Additionally, Social Links has been merged with no mover controls. This PR resurfaces them.
  • It restyles, positions, and rotates the mover control to be horizontal.
  • It restyles and positions the ellipsis menu.

The overall pattern it mimics, is that of gallery items, which also have horizontal movers, even though those are not yet technically child blocks.

Screenshot 2019-09-27 at 10 30 25

One of the things I think still needs a little thought, is the "selected block style". Specifically for Social Links, when you click the button to modify it, you're clicking the button inside the block. Technically that also highlights the block, but it's the button that has focus.

Your feedback is much apprecated.

GIF:

horizontal movers

This is more of a proof of concept and experiment, than it is final PR. So *do not merge as is*.

We might find that we like this, in which case it might need a little polish, and _then_ we can ship it. But for now this PR has been created to help further discussion in #16637.

This PR does the following:

- Starts with Social Links, because that is the first block to "absorb child block UI", and therefore a good test-bed for this.
- Additionally, Social Links has been merged with _no mover controls_. This PR resurfaces them.
- It restyles, positions, and rotates the mover control to be horizontal.
- It restyles and positions the ellipsis menu.

The overall pattern it mimics, is that of gallery items, which also have horizontal movers, even though those are not yet technically child blocks.

Your feedback is much apprecated.
@draganescu

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

@mtias and I have talked about such an approach and the pain points are:

  • movers on top of another block make the block below harder to select (basically you have to deselect the selected one if the block below is small enough)
  • movers on top of the block they move are ok, except for text based blocks for which the mover on top makes the text harder to edit, as you need to click so that you don't activate de mover

This is where inline movers solve both issues.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

Thanks for the clarification. I tend to agree with that, but I'm also always looking for interim solutions in the here and now, which can move us forward in small steps. That's not necessarily an argument for this PR landing, but hopefully actual code can grease the discussion, i.e. now we know how those overlay movers feel!

Edit: and for what it's worth, I expected the small hit area and the fact that it overlaid neighboring blocks to be the primary problem. I don't feel that's it at all, I feel it's the fact that as you click the button, the block moves is the problem, so you can't just "click click click".

@draganescu

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

I don't feel that's it at all, I feel it's the fact that as you click the button, the block moves is the problem, so you can't just "click click click".

OH YES :)

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

I would think that the movers and ellipsis menu should be placed above the blocks, not to the sides. Having the movers on top is an existing pattern used by full-aligned blocks, and having the ellipsis menu on top is just consistent with every other block toolbar. Also, having the controls on top makes it easier to select the adjacent blocks on the left and right.

@jasmussen jasmussen referenced this pull request Oct 1, 2019
3 of 5 tasks complete
Copy link
Member

left a comment

Hi @jasmussen this PR seems to work well 👍
I only noticed a small regression the block ellipsis menu becomes visible in this PR: making it impossible to click on the side block:
Screenshot 2019-10-02 at 20 02 54

I like the fact that this UI does not increases the block spacing, making the social links even when selected look the same as in the frontend.

I think for the long term we need an API that allows the core to deal with horizontal blocks instead of forcing all the blocks that want horizontal movers to add these styles. An API like this is being implemented at #16615. But I think it should be experimental for now until we apply it to more blocks and add other customization options. Probably will not be part of WordPress 5.3.

If the social links block is going to be included by default in WordPress 5.3, It would be good that the block has movers. This PR seems like a possible solution to get the movers on social links in WordPress 5.3, given that it has no changes in the API.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

Why can't the block movers just look like this:

image

Also, the heavy left block border looks odd when there is no block title or toolbar above it. It feels especially awkward with the movers sticking out but not appearing quite attached on one side, and the ellipsis appearing on the other side of the block where there is no visible border.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

Thanks for the review, @jorgefilipecosta.

I created this PR mostly as a proof of concept. I think @youknowriad has some ideas around how to take this to the next step, and had some good reasons for why it might need to be marked as experimental for 5.3, which I'd be fine with. But I'm glad you like how the CSS works. I strongly agree that the CSS rules here should be extracted into a prop so any block can use it for nested elements. In fact since creating this PR, I have pushed a better version of the rules to #16637, so that might serve as a better base for such a PR.

If Riad thinks we can ship Social Links not as experimental, I can scramble and address your points. Otherwise I think this PR is good to serve just as an alternate approach for how to integrate horizontal movers, and just be an experiment.

Zeb:

Why can't the block movers just look like this:

They can, do you think that's better?

I'd personally like to explore a better interface for drag and drop, by the way. I think #17088 may provide the ingredients to make that work.

Also, the heavy left block border looks odd when there is no block title or toolbar above it.

Agree. I think this needs a larger effort than just here.

One thing that has become clear, working on this, is that we might need an indicator for when the block itself has focus, and when an element inside the block has focus. In the case of social links, when you click the child block you actually focus the link itself, which causes the link dialog to pop out. But you might expect that the child block itself is also selected, suggesting you could press Delete to remove it. That's the reason for keeping the block borders, in addition to the focus styles that are present. But yes, I think they can be refined, but worth doing globally.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

The main reason for the social links block to be experimental is to give us more time to ship it in WP 5.4 instead of 5.3. So let's iterate on it and improve it.

@draganescu

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

@jasmussen did you get a chance to look at #17701 which implements horizontal movers provided by the new property of InnerBlocks? I could use your take on that too :)

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

@jasmussen

Why can't the block movers just look like this:

They can, do you think that's better?
Yeah, I think that's better.

Also, I definitely agree that the Social Links block should be made experimental.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.