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: Larger drag handles #10331

Merged
merged 5 commits into from Oct 4, 2018

Conversation

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Oct 4, 2018

This is an attempt to make a larger target area for drag handles. The handle itself doesn't change its appearance, but you now have a larger target area for your cursor, as depicted here:

fullscreen_4_10_18__10_49

Basically the visible handle is now a pseudo element positioned inside the larger handle container.

Fixes #8206 — the larger target area makes it a bit easier to grab the handle when there's a block below the spacer block.

I'd like to also extend this to the image block but just want the OK before going forward with that.

Some other notes:

I introduced a new variable, $resize-handler-size: 16px, because until now the spacer and image drag handles were different sizes (15px and 16px respectively). If this approach is worth moving forward with, I'll use this variable on the image drag handles as well.

I am also using another new variable $resize-handler-container-size to calculate the size of the container (the drag handle plus padding). I'm doing it inline in the spacer block drag handle CSS block but if this moves forward I would move it in with the other variables. This is just temporary :)

@chrisvanpatten chrisvanpatten requested review from tofumatt, jasmussen and WordPress/gutenberg-core Oct 4, 2018
Copy link
Member

tofumatt left a comment

Tested locally and this works for me. I like it.

Once the SCSS is tidied up a bit I say we ship it.

position: absolute;
background: theme(primary);
left: calc(50% - #{$resize-handler-container-size / 2}) !important;

This comment has been minimized.

Copy link
@tofumatt

tofumatt Oct 4, 2018

Member

!important 😱

For real though, why are these here? We shouldn't need them, and if we really do we should have comments.

(But we shouldn't need them 😄)

This comment has been minimized.

Copy link
@gwwar

gwwar Oct 4, 2018

Contributor

+1 on avoiding !important when we're able to. I see that there's an inline style that includes a left rule. If possible could we try to update the related resizing JS to take this in account instead?

Adding !important rules makes this incredibly difficult to override, folks that would like other styles, basically end up needing to add another !important rule, and hope that the rule is read last.

This comment has been minimized.

Copy link
@chrisvanpatten

chrisvanpatten Oct 4, 2018

Author Member

@gwwar @tofumatt Agreed, but the resize component we're using here applies inline styles which we need to override. However I also see that they support a style prop so I'll see if I can just disable the inline styles.

bottom: calc(#{$resize-handler-container-size / 2} * -1) !important;

// Apply the width/height of the container size plus padding so the handle
// sits in the middle of the grabbable area

This comment has been minimized.

Copy link
@tofumatt

tofumatt Oct 4, 2018

Member

Nitpick but it'd be nice if all of these comments ended in fullstops. I wish we had a linting rule for that! 😓

This comment has been minimized.

Copy link
@chrisvanpatten

chrisvanpatten Oct 4, 2018

Author Member

I like nitpicks 😜 I'll get it updated! Agreed, a linting rule would be nice, but maybe this could be put into contributing docs or something too?

This comment has been minimized.

Copy link
@tofumatt

tofumatt Oct 4, 2018

Member

I would approve such a pull request! 😄

Copy link
Member

karmatosed left a comment

I think this is a good addition, thanks!

@chrisvanpatten

This comment has been minimized.

Copy link
Member Author

chrisvanpatten commented Oct 4, 2018

@tofumatt Unfortunately I was not able to remove the !important. I tried passing an empty object, false, and null to the ResizableBox component and the styles persisted.

So, in lieu of that, I wrote a bit of documentation and linked back to this ticket!

A few other tweaks:

  • Moved the container size variable into edit-post/assets/stylesheets/_variables.scss
  • Added mixins for the resize handle container and visible handle
  • Applied those mixins in the spacer block
  • Updated the image block to use the new mixins
  • Successfully tested the image block in various configurations (different alignments and such)

🚀

Copy link
Member

tofumatt left a comment

😭 Arg, so it's an upstream issue.

Can we file/find/link to an upstream issue that would allow us to override/disable their styles so we can avoid the !important usage?

After that I would... probably begrudgingly accept this 😅

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 4, 2018

Tried checking this out, and not really seeng any visual difference:

screen shot 2018-10-04 at 14 09 56

screen shot 2018-10-04 at 14 09 43

Which is the point, of course. And in that vein, ship it!

The bottom drag handle does sort of sit at the same spot where the sibling inserter is, which could be better. But that's a separate issue.

@chrisvanpatten

This comment has been minimized.

Copy link
Member Author

chrisvanpatten commented Oct 4, 2018

@jasmussen I tracked that in #8206. It’s a little annoying but now there’s at least slightly more space to grab onto!

@tofumatt tofumatt added this to the 4.0 milestone Oct 4, 2018
@tofumatt

This comment has been minimized.

Copy link
Member

tofumatt commented Oct 4, 2018

I know 235e8dc is more code but I super-appreciate the lack of !importants! ❤️❤️❤️

@chrisvanpatten

This comment has been minimized.

Copy link
Member Author

chrisvanpatten commented Oct 4, 2018

@tofumatt Okay I think I got it now! I couldn't remove the entire style definition, but I could remove the properties individually. So we can now avoid !important here!

I'm pretty sure spacers and images are the only uses of drag handles in Gutenberg core blocks but if I'm wrong definitely let me know 🤘

A future improvement here might be to ship our own ResizableBox that extends re-resizable with shared class names across all block implementations rather than each block having its own block-specific resize handle classname. We could also probably simplify the API a bit with a wrapper. But I don't think that's necessary to tackle here and would require some other decisions that I'm not qualified to make :)

@chrisvanpatten

This comment has been minimized.

Copy link
Member Author

chrisvanpatten commented Oct 4, 2018

@tofumatt You beat me by a hair ;-) But yes, I agree — no !important is worth it!

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 4, 2018

I'm pretty sure spacers and images are the only uses of drag handles in Gutenberg core blocks but if I'm wrong definitely let me know 🤘

They will also be used in #9416 by @jorgefilipecosta

Copy link
Member

tofumatt left a comment

Sooooo, this is good but given we reproduce this bit of code twice, maybe we want a common component that is a <ResizableBox> with the style overrides? I could see this happening again without someone catching it.

I think this is worth shipping now but can you file a follow-up issue to combine these? Actually, maybe it wouldn't make sense as the overrides vary? I dunno, if you think it's worth combining them a follow-up code quality issue would be cool. If you think it's not worth it that's fine.

@chrisvanpatten

This comment has been minimized.

Copy link
Member Author

chrisvanpatten commented Oct 4, 2018

@jasmussen Ooh, thanks for the heads up.

@tofumatt Yeah I think it's worth following this up with a "create our own wrapper around ResizableBox" PR, but I'd like to handle (heh) that in a separate issue :)

@chrisvanpatten chrisvanpatten merged commit b09005c into WordPress:master Oct 4, 2018
2 checks passed
2 checks passed
codecov/project 49.21% (-0.18%) compared to 0332aa9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chrisvanpatten chrisvanpatten deleted the TomodomoCo:try/bigger-drag-handles branch Oct 4, 2018
@chrisvanpatten

This comment has been minimized.

Copy link
Member Author

chrisvanpatten commented Oct 4, 2018

Thanks for the support and guidance on this everyone!

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.