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

Make drag handle visible on hover in nested #10063

Merged
merged 4 commits into from Sep 21, 2018

Conversation

Projects
None yet
3 participants
@jasmussen
Contributor

jasmussen commented Sep 20, 2018

In nested contexts, the drag handle doesn't have a background on hover like the up/down arrows. This PR refactors things a bit and fixes it up.

Before:

screen shot 2018-09-20 at 11 43 20

After:

screen shot 2018-09-20 at 12 05 25

Make drag handle visible on hover in nested
In nested contexts, the drag handle doesn't have a background on hover like the up/down arrows. This PR refactors things a bit and fixes it up.

@jasmussen jasmussen added this to the 4.0 milestone Sep 20, 2018

@jasmussen jasmussen self-assigned this Sep 20, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Sep 20, 2018

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 20, 2018

Contributor

Thanks, Joen — this is looking good, but I wonder if we can adjust the spacing a little bit before merging? Right now the top of the drag handle's top border overlaps with the bottom border of the up arrow, and the bottom border bumps right up against the top of the down arrow. It looks a little imbalanced:

screen shot 2018-09-20 at 10 52 46 am

It'd be great if there was a 1px border in between all:

1px

Contributor

kjellr commented Sep 20, 2018

Thanks, Joen — this is looking good, but I wonder if we can adjust the spacing a little bit before merging? Right now the top of the drag handle's top border overlaps with the bottom border of the up arrow, and the bottom border bumps right up against the top of the down arrow. It looks a little imbalanced:

screen shot 2018-09-20 at 10 52 46 am

It'd be great if there was a 1px border in between all:

1px

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 21, 2018

Contributor

It frustrates me a little bit that we want to target nested blocks separately. I mean I understand why, I just feel like there has to be a middle road that works in both contexts and feels a little less kludgy.

One concept is something like this that treats the entire mover as single area, like so:

edit_post_ tomodomo _wordpress

(Note that in the screenshot I did also align the top edge of the movers and the block canvas. I have a lot of reasons why I think that's right, which I spent some time typing up and then decided not to share them so as not to derail my main point about nested/non-nested consistency. So ignore the vertical positioning, and just pay attention to the single border rather than individual button borders. 😄)

Contributor

chrisvanpatten commented Sep 21, 2018

It frustrates me a little bit that we want to target nested blocks separately. I mean I understand why, I just feel like there has to be a middle road that works in both contexts and feels a little less kludgy.

One concept is something like this that treats the entire mover as single area, like so:

edit_post_ tomodomo _wordpress

(Note that in the screenshot I did also align the top edge of the movers and the block canvas. I have a lot of reasons why I think that's right, which I spent some time typing up and then decided not to share them so as not to derail my main point about nested/non-nested consistency. So ignore the vertical positioning, and just pay attention to the single border rather than individual button borders. 😄)

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 21, 2018

Contributor

Great catches. I addressed the feedback:

screen shot 2018-09-21 at 10 08 41

The borders are stacked rather than space between, I have a slight preference for that.

It also works when you multiselect in a nested context:

screen shot 2018-09-21 at 10 07 47

The meta issue here, as beautifully noted and addressed by @chrisvanpatten, is that this isn't ideal. I totally agree, but thankfully I feel we've made strides by moving the ellipsis to the block toolbar and simplified things like the drag handle, at least it's getting there.

I also happen to prefer the mockup Chris provided, in fact I tried doing something like that, but the current setup doesn't easily lend itself to this design. That doesn't mean it shouldn't be done, just that I couldn't do it fast, and I'd rather have small iterative improvements in the mean time.

Contributor

jasmussen commented Sep 21, 2018

Great catches. I addressed the feedback:

screen shot 2018-09-21 at 10 08 41

The borders are stacked rather than space between, I have a slight preference for that.

It also works when you multiselect in a nested context:

screen shot 2018-09-21 at 10 07 47

The meta issue here, as beautifully noted and addressed by @chrisvanpatten, is that this isn't ideal. I totally agree, but thankfully I feel we've made strides by moving the ellipsis to the block toolbar and simplified things like the drag handle, at least it's getting there.

I also happen to prefer the mockup Chris provided, in fact I tried doing something like that, but the current setup doesn't easily lend itself to this design. That doesn't mean it shouldn't be done, just that I couldn't do it fast, and I'd rather have small iterative improvements in the mean time.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 21, 2018

Contributor

@jasmussen agreed wholeheartedly. I have a WIP branch with several other tweaks that I can push up but that shouldn’t block this. I say 🚢 it!

Contributor

chrisvanpatten commented Sep 21, 2018

@jasmussen agreed wholeheartedly. I have a WIP branch with several other tweaks that I can push up but that shouldn’t block this. I say 🚢 it!

@kjellr

kjellr approved these changes Sep 21, 2018

🚢 Thanks, @jasmussen!

I also like that exploration by @chrisvanpatten — it'd be great to see something like that happen in the future.

@jasmussen jasmussen merged commit 2cd23ca into master Sep 21, 2018

2 checks passed

codecov/project 48.67% (-0.02%) compared to d5bbd81
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the fix/drag-handle-nested branch Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment