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

Move and resize line shapes #2594

Merged
merged 12 commits into from May 9, 2018

Conversation

Projects
None yet
3 participants
@rmoestl
Copy link
Contributor

commented May 1, 2018

This implements a more intuitive movement and resizing of line shapes specified in #2038. Please refer to the commit message for a rough overview of the feature.

In addition renamed image test 'fixed_size_shapes' to 'shapes_fixed_size' to better fit the de-factor naming convention for image tests.

rmoestl added some commits Apr 30, 2018

Implement more intuitive line shapes movement and resizing [2038]
- Reason: until now line shapes were treated the same as
  rectangles and circles with the result that a line's
  start or end point couldn't be dragged freely but only
  in the quadrant where it was located.
- Use of invisible helper elements to make grabbing a
  line and its ends easier.
- Adapt shapes's Jasmine tests and add new ones.
- Add an (editable) image test serving as an interactive
  testbed.
Rename image test 'fixed_size_shapes' to 'shapes_fixed_size'
- Reason: to better fit the de-facto naming convention for image tests
  which is starting with the plot type.

@rmoestl rmoestl self-assigned this May 1, 2018

@rmoestl rmoestl requested a review from alexcjohnson May 1, 2018

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

Sweet! I haven't looked at the code yet, just playing with your awesome shapes_move-and-reshape-lines mock 🎉 The mechanics of it are great! But I noticed a few things:

  • I like the move cursor over the middle section, but how about grab for the ends? Won't work in IE but I think that's OK, it'll fall back to default there I guess, which is what you use now.
  • Not part of this PR but I also notice we don't do anything special with the cursor when an annotation is draggable. It's a little tricky what to do with the text box, as click edits the text and drag moves it... but the arrow itself could safely get a move cursor. Your call whether to add that to this PR or make a new issue for it.
  • When you drag a shape off the edge of an autoranged plot, dragging still works but part of the shape is hidden by the clip-path - could be helpful to remove the clipping during the move. I guess to be precise about it, if one of the axes is autoranged and the other is fixed, we should only stop clipping on the autoranged axis. Note that there are single-axis clipPaths already defined and available for this purpose.
  • Also maybe not part of this PR, but came up while I was playing: seems like there's something off with y-autorange + fixed-size shapes. When one end of the green line is highest, the autorange is about right, but when the other end is highest it reserves too much space. Does not seem to happen in the x direction:
    screen shot 2018-05-01 at 9 33 26 am
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

Good call renaming 'fixed_size_shapes' to 'shapes_fixed_size' - I hadn't noticed that but yes, it's nice to keep all the shapes mocks together in our various listings.

xa = Axes.getFromId(gd, shapeOptions.xref);
ya = Axes.getFromId(gd, shapeOptions.yref);
function createLineDragHandles() {
var minSensoryWidth = 10,

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson May 1, 2018

Contributor

Might want to move this into https://github.com/plotly/plotly.js/blob/master/src/constants/interactions.js?

This behavior for annotations is even more ugly and hard-coded

.attr({
d: 'M3,3H-3V-3H3ZM0,0L' + (tailX - arrowDragHeadX) + ',' + (tailY - arrowDragHeadY),
transform: 'translate(' + arrowDragHeadX + ',' + arrowDragHeadY + ')'
})
.style('stroke-width', (strokewidth + 6) + 'px')

It wouldn't need to be changed now, but if we were to move this to a constants file and reuse it in annotations, I like the Math.max logic you have here better than the width + 6 currently in annotations (though the little-bit-extra around the head should be preserved).

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson May 1, 2018

Contributor

Per @etpinard 's comment below, feel free to defer all of ^^ to #679

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

Oh hmm, re: the y-autorange issue: I think I just got the anchor point outside the actual range of the line, so it was autoranging to the anchor point, which is totally reasonable! I'm not really sure how to handle this, but as it is it's too easy to get into strange unintuitive situations, since there's nothing to show you visually where the anchor point is.

The easy way to handle this would be just not to allow GUI resizing of pixel-sized shapes, only moving them. Or perhaps allow resizing but not past the anchor point, so the shape always contains the anchor? That would cause problems for lines though, as you'd have the same problem this PR is intended to fix, of not being able to change the direction the way you want to. A more complete solution might be to show the anchor point during the resize - that would let people see what they're really doing. Perhaps a crosshair, white with a black outline like we make for zoom box corners?

@etpinard

This comment has been minimized.

Copy link
Member

commented May 1, 2018

but I also notice we don't do anything special with the cursor when an annotation is draggable

That one has its own issue already -> #679

@rmoestl

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2018

Re: cursors for annotations

but the arrow itself could safely get a move cursor

Done.

It's a little tricky what to do with the text box, as click edits the text and drag moves it...

What about the text or pointer cursor for the text and the move cursor for the rect around the text? E.g. Trello is using text and pointer cursors to indicate direct editing of card and list titles.

@rmoestl

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2018

Re: auto-range, anchor points and fixed size shapes, in my opinion showing the anchor point (and perhaps a thin line connecting the shape with the anchor point) would be the way to go.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented May 2, 2018

What about the text or pointer cursor for the text and the move cursor for the rect around the text? E.g. Trello is using text and pointer cursors to indicate direct editing of card and list titles.

Currently the two behave the same when you click or drag them, I think - so I'd keep the cursor the same for both elements. Maybe pointer is best to capture the multiple possibilities? I suppose we could also use text but change it to move once you start dragging? Dunno, I don't have a strong opinion.

rmoestl added some commits May 2, 2018

Adapt cursors for annotation interactions
- Reason: in conjunction with moving shapes [2038], to get a more
  consistent cursor behavior for movable elements.

Use 'move' cursor for movable annotation arrows

- Reason: in conjunction with moving shapes [2038], to get a more
  consistent cursor behavior for movable elements.
Deactivate clip path temporarily while dragging shapes [2038]
- Hint: only deactivate for auto-ranged axes.
- Reason: to allow for more accuracy when dragging shapes outside
  of the plot's bounds.
Render anchor point when moving a pixel sized shape [2038]
- Reason: resizing a pixel sized shape is changing the distance to its
  anchor point. The anchor point not being visible caused
  misleading perceptions in case of auto-ranged axis because the
  auto-range algorithm takes both anchor point and pixel coordinates of
  shape into account.
- Even works for `path` shapes and shapes where only one dimension
  is pixel sized.
Disable resize cursors for path shapes
- Reason: resize cursors were shown although resizing path shapes
  is not yet supported.
Correct baseline image for shapes_move-and-reshape-lines image test
- Reason: have forgotten to update the baseline image after correcting
  a typo in an annotation.

@etpinard etpinard added this to the v1.38.0 milestone May 3, 2018

@rmoestl

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2018

Short comment on showing the anchor point: dropped the idea to additionally display a thin line. IMO showing the cross-hair is enough of a visual cue. Felt quite natural to me, please use https://github.com/plotly/plotly.js/blob/move-and-resize-line-shapes/test/image/mocks/shapes_fixed_size.json to see how it behaves in different edge cases.

Also, in case there's a place in the code to gather functions together that generate some kind of "symbol" like the cross-hair shape, please let me know. Then I'd move the cross-hair generating code to this place.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

dropped the idea to additionally display a thin line. IMO showing the cross-hair is enough of a visual cue. Felt quite natural to me

Agreed, this feels very clear to me as you have it now, makes it really obvious what's going on and how to get the alignment you want. Nice! Thanks for adding the annotation cursors as well, make sure you close #679 in addition to #2038 when this is merged 🎉

I haven't looked at the code yet, but two comments on the mechanics:

  • The cross gets blurry much of the time. .crisp can help, though even with that I've seen cases where corner pixels get dropped unexpectedly. The best solution is to round its positioning to exact half-integer pixels - hence all the .5's here - but that one doesn't need to explicitly round because it's positioned based on a mouse event... yours is placed based on data.

only a little blurry:
less blurry

very blurry:
blurry

  • I'm not quite sure what to do with the (very unusual, but supported) case of one axis fixed-pixels and the other data- or plot-referenced... Having the anchor cross appear at one edge of the non-pixel dimension feels weird though. Perhaps it would be enough to put it at the center of that dimension? Even better might be to put it at the center AND make it just a bar - a horizontal bar if only the y direction is fixed pixels, a vertical bar if only x is fixed. Like we do for 1D zooms:
    screen shot 2018-05-08 at 11 38 34 am 2

Also, in case there's a place in the code to gather functions together that generate some kind of "symbol" like the cross-hair shape, please let me know.

No, not at present. We do have nearly a identical path defined in ternary. That one is smaller than the one you made, which probably makes sense, so I'm not sure DRYing this up is a good goal, though there would be something to be said for collecting these paths at least to ensure the differences between them are intentional. Anyway I wouldn't worry about that in this PR but it's a good point to keep in mind for later.
screen shot 2018-05-08 at 11 28 35 am 2

function setClipPath(shapePath, gd, shapeOptions) {
// note that for layer="below" the clipAxes can be different from the
// subplot we're drawing this in. This could cause problems if the shape
// spans two subplots. See https://github.com/plotly/plotly.js/issues/1452

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson May 8, 2018

Contributor

That issue is closed - is there still a case where this currently breaks? I see #1452 (comment) about layering and inset plots, is that what you're referring to?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson May 8, 2018

Contributor

Oh sorry, you just copied this comment on refactor. NVM!

@@ -232,7 +233,7 @@ function setupDragElement(gd, shapePath, shapeOptions, index, shapeLayer) {
h = dragBBox.bottom - dragBBox.top,
x = evt.clientX - dragBBox.left,
y = evt.clientY - dragBBox.top,
cursor = (w > MINWIDTH && h > MINHEIGHT && !evt.shiftKey) ?
cursor = (!isPath && w > MINWIDTH && h > MINHEIGHT && !evt.shiftKey) ?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson May 8, 2018

Contributor

nice catch - and thanks for updating the comment below!

@rmoestl

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

I'm not quite sure what to do with the (very unusual, but supported) case of one axis fixed-pixels and the other data- or plot-referenced

Me neither 😄. I was thinking about that case. My second option would have been a bar with the full length of the not-pixel-sized dimension of the shape. But then came to the conclusion that this would probably confuse people.

My rationale for the current solution is that (i) positioning the anchor cross on the x0 / y0 coordinate of the not-pixel-sized dimension gives the user a useful hint where the shape "starts" and (ii) drawing a cross even in this case is a more consistent user experience.

Guess, it's best to test it out.

Draw bar instead of anchor cross when moving hybrid-sized shapes [2038]
- When moving a shape that is pixel-sized in one dimension and
  data-sized or paper-sized in the other dimension a centered vertical
  or horizontal bar is rendered instead of the anchor cross.
- Thereby fix a bug in `extractPathCoords` to return numerical values
  instead of numbers represented as strings.
- Also introduce the statistical `midRange` function in lib.
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

Nicely done. I like it with the small centered bars for the one-axis-fixed-pixels case. Now that they're centered, rather than sitting on one (seemingly arbitrary) edge, I could see going back to a cross, but to my eye anyway it's a little bit clearer and more intuitive as a bar.

The drawing is very crisp now too, thanks for cleaning that up. I think we're ready! 💃

@rmoestl rmoestl merged commit 2535b1a into master May 9, 2018

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@rmoestl rmoestl deleted the move-and-resize-line-shapes branch May 9, 2018

@rmoestl rmoestl removed their assignment May 9, 2018

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