Move and resize line shapes #2594

rmoestl · 2018-05-01T07:34:39Z

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.

- 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.

- Reason: to better fit the de-facto naming convention for image tests which is starting with the plot type.

alexcjohnson · 2018-05-01T13:39:38Z

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:

alexcjohnson · 2018-05-01T13:43:18Z

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.

alexcjohnson · 2018-05-01T14:07:01Z

src/components/shapes/draw.js

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


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

plotly.js/src/components/annotations/draw.js

Lines 536 to 540 in 2b1713d

.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).

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

alexcjohnson · 2018-05-01T14:32:27Z

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 · 2018-05-01T20:33:04Z

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 · 2018-05-02T12:33:13Z

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 · 2018-05-02T12:37:50Z

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 · 2018-05-02T13:02:55Z

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.

- 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.

- Hint: only deactivate for auto-ranged axes. - Reason: to allow for more accuracy when dragging shapes outside of the plot's bounds.

- 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.

- Reason: resize cursors were shown although resizing path shapes is not yet supported.

- Reason: have forgotten to update the baseline image after correcting a typo in an annotation.

rmoestl · 2018-05-08T14:32:12Z

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 · 2018-05-08T15:41:35Z

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:

very 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:

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.

alexcjohnson · 2018-05-08T15:50:03Z

src/components/shapes/draw.js

+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


~~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?~~

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

alexcjohnson · 2018-05-08T15:56:23Z

src/components/shapes/draw.js

@@ -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) ?


nice catch - and thanks for updating the comment below!

rmoestl · 2018-05-09T07:55:24Z

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.

- 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 · 2018-05-09T12:44:32Z

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 added 2 commits May 1, 2018 09:16

Rename image test 'fixed_size_shapes' to 'shapes_fixed_size'

88d959d

- Reason: to better fit the de-facto naming convention for image tests which is starting with the plot type.

rmoestl self-assigned this May 1, 2018

rmoestl requested a review from alexcjohnson May 1, 2018 07:34

alexcjohnson reviewed May 1, 2018

View reviewed changes

etpinard added feature status: reviewable labels May 1, 2018

Use 'grab' cursor when moving either end of a line shape [2038]

dffe308

rmoestl added 6 commits May 2, 2018 16:21

Deactivate clip path temporarily while dragging shapes [2038]

7729133

- Hint: only deactivate for auto-ranged axes. - Reason: to allow for more accuracy when dragging shapes outside of the plot's bounds.

Fix typo in shapes_move-and-reshape-lines.json image test [2038]

116d53f

Disable resize cursors for path shapes

ae46cea

- Reason: resize cursors were shown although resizing path shapes is not yet supported.

Correct baseline image for shapes_move-and-reshape-lines image test

f2c9a4c

- Reason: have forgotten to update the baseline image after correcting a typo in an annotation.

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

alexcjohnson reviewed May 8, 2018

View reviewed changes

rmoestl added 2 commits May 9, 2018 08:52

Ensure cross-hair rendered upon moving pixel sized shape is sharp [2038]

c806458

Simplify d attr of anchor cross for pixel-sized shapes [2038]

c686473

rmoestl merged commit 2535b1a into master May 9, 2018

rmoestl deleted the move-and-resize-line-shapes branch May 9, 2018 14:24

rmoestl mentioned this pull request May 9, 2018

Implement setCursor handler for editable annotations #679

Closed

rmoestl removed their assignment May 9, 2018

rmoestl mentioned this pull request May 9, 2018

Ability to interactively change length and rotate line shapes #2038

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Move and resize line shapes #2594

Move and resize line shapes #2594

rmoestl commented May 1, 2018

alexcjohnson commented May 1, 2018 •

edited by rmoestl

Loading

alexcjohnson commented May 1, 2018

alexcjohnson May 1, 2018

alexcjohnson May 1, 2018

alexcjohnson commented May 1, 2018

etpinard commented May 1, 2018

rmoestl commented May 2, 2018 •

edited

Loading

rmoestl commented May 2, 2018 •

edited

Loading

alexcjohnson commented May 2, 2018

rmoestl commented May 8, 2018 •

edited

Loading

alexcjohnson commented May 8, 2018

alexcjohnson May 8, 2018 •

edited

Loading

alexcjohnson May 8, 2018

alexcjohnson May 8, 2018

rmoestl commented May 9, 2018

alexcjohnson commented May 9, 2018

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

Move and resize line shapes #2594

Move and resize line shapes #2594

Conversation

rmoestl commented May 1, 2018

alexcjohnson commented May 1, 2018 • edited by rmoestl Loading

alexcjohnson commented May 1, 2018

alexcjohnson May 1, 2018

Choose a reason for hiding this comment

alexcjohnson May 1, 2018

Choose a reason for hiding this comment

alexcjohnson commented May 1, 2018

etpinard commented May 1, 2018

rmoestl commented May 2, 2018 • edited Loading

rmoestl commented May 2, 2018 • edited Loading

alexcjohnson commented May 2, 2018

rmoestl commented May 8, 2018 • edited Loading

alexcjohnson commented May 8, 2018

alexcjohnson May 8, 2018 • edited Loading

Choose a reason for hiding this comment

alexcjohnson May 8, 2018

Choose a reason for hiding this comment

alexcjohnson May 8, 2018

Choose a reason for hiding this comment

rmoestl commented May 9, 2018

alexcjohnson commented May 9, 2018

alexcjohnson commented May 1, 2018 •

edited by rmoestl

Loading

rmoestl commented May 2, 2018 •

edited

Loading

rmoestl commented May 2, 2018 •

edited

Loading

rmoestl commented May 8, 2018 •

edited

Loading

alexcjohnson May 8, 2018 •

edited

Loading