Skip to content

Fixed size shapes #2532

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 11, 2018
Merged

Fixed size shapes #2532

merged 8 commits into from
Apr 11, 2018

Conversation

rmoestl
Copy link
Contributor

@rmoestl rmoestl commented Apr 6, 2018

This implements fixed size shapes specified in #2193. Please refer to the commit message for a rough overview of the feature.

rmoestl added 3 commits April 6, 2018 14:31
@rmoestl
Move assertions from hover_label_test.js to custom_assertions.js #2193
dc2f3ba 
- Reason: prepare writing fixed size shape tests.
@rmoestl
Enhance documentation of autorange.js expand function
49f7fb0
@rmoestl
Introduce fixed size shapes #2193
d386b2d 
- Shapes (line, rect, circle as well as path) can now be
  sized by pixel while being positioned relative to data.
- New shape attributes `xsizemode`, `xanchor`, `ysizemode`
  and `yanchor` introduced and semantics of `x0`, `x1`,
  `y0` and `y1` extended.
- Shapes can be sized by pixel on one axis and sized by
  data on the other.
- Fixed size shapes can be moved.
- Fixed size lines, rectangles and circles can be resized,
  paths not.
- Fixed size shapes are fully taken into account when an
  axis is in auto-range mode.
@rmoestl rmoestl self-assigned this Apr 6, 2018
@rmoestl rmoestl requested review from alexcjohnson and etpinard April 6, 2018 13:42
alexcjohnson
alexcjohnson reviewed Apr 6, 2018
View reviewed changes
src/components/shapes/attributes.js Outdated
xsizemode: {
valType: 'enumerated',
values: ['data', 'pixel'],
dflt: 'data',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I hadn't thought of this before, but 'data' mode applies not just to axis scaling but also plot fraction scaling, right? ie when xref='paper' this option is still relevant, right? So how about we change 'data' to 'scaled'? That seems like it encompasses both cases and differentiates them from 'pixel' sizing...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll change that.

@alexcjohnson
Copy link
Collaborator

@rmoestl this is looking really great! I think there are enough new cases, re: axis vs paper referencing, autorange, and paths vs other shape types, that it deserves a new test image - similar to the shapes mock, so we can see that the autorange results are correct and the shapes are drawn as expected. That would also help me to try out the new dragging effects and see if our conclusion about edge vs center dragging really holds up 😄

rmoestl added 2 commits April 10, 2018 20:06
@rmoestl
Rename 'data' to 'scaled' for size mode attrs of fixed size shapes #2193
c2203fa
@rmoestl
Add an image test for fixed size shapes #2193
ce9691c 
- Note: not an exhaustive mock with all variations but rather a set of
  shapes covering the newly introduced capabilities, also useful
  for manually testing and debugging things.
@rmoestl
Copy link
Contributor Author

rmoestl commented Apr 10, 2018

The new test image is here: https://github.com/plotly/plotly.js/blob/fixed-size-shapes/test/image/baselines/fixed_size_shapes.png

alexcjohnson
alexcjohnson reviewed Apr 10, 2018
View reviewed changes
test/image/mocks/fixed_size_shapes.json
"config": {
"editable": "true"
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice test image, and I think the interactions feel very natural as you have them. Other than adding a trailing newline to make the syntax test happy the only change I'd like to see is to use the other three axis types (log, date, category) on one axis each - no need to add more subplots, just convert a couple of the existing axes.

@alexcjohnson
Copy link
Collaborator

One thing I just noticed is the cursors start out wrong (at least in some cases) - the "top" cursor shows for most (or all!) of the shape, then after you cause a redraw (in the gif below I zoom in then double click to autorange again) the cursors look correct.

I haven't looked to see if this is exclusive to pixel-sized shapes, nor if it's new with this PR... can you check it out @rmoestl ?

weird shapes cursor

@rmoestl
Copy link
Contributor Author

rmoestl commented Apr 11, 2018

I'll check out the cursor position thing.

@rmoestl
Copy link
Contributor Author

rmoestl commented Apr 11, 2018

Re: drag cursors, unfortunately I was not yet able to reproduce this strange behavior.

Looking at the code, the bounding box of the shape is obtained in setupDragElement and later used in the mouse move event handler to calculate the cursor. Can resizing cause this behavior? I don't think so because a resize triggers drawing the shape which in turn triggers a new call of setupDragElement. In addition your recording shows that you've not yet resized the respective shape.

Do you have some hints on how to reproduce the bug? I was testing in Chrome and FF by the way. Though I assume getBoundingClientRect is working correct in all major browsers.

@alexcjohnson
Copy link
Collaborator

Thanks for the test image tweaks - love it!

I figured out what the bug is - it's when you scroll the page after the plot draws (which I was doing naturally with your mock because it's fairly tall, whereas the original shapes mock is much shorter - if you shrink your browser window so you need to scroll that one you can see it there too).

It's not new in this PR, but I think it has a simple solution so you might as well address it here. The issue is that getBoundingClientRect gets called in the outer scope of setupDragElement, and gets used later in updateDragMode, at which point the actual bbox is different if you've scrolled. The simple solution is to move getBoundingClientRect to the scope where it's used.

@rmoestl
Fix drag cursor bug for shapes
eb853ab 
- Bug revealed during review of #2193
- Bug: if a shape is not or not completely on screen at time of drag
  element setup, obtained bounding box would not be correct leading
  to the calculation of a wrong cursor once the shape scrolls into the
  viewport.
@rmoestl
Copy link
Contributor Author

rmoestl commented Apr 11, 2018

Wow, good catch!!

@etpinard etpinard added this to the v1.36.0 milestone Apr 11, 2018
@alexcjohnson
Copy link
Collaborator

Great, glad that's all it took! That bug fix may actually help performance too - ironically, since now we're calling getBoundingClientRect many more times, but we're not calling it during draw, only during mouse moves.

I'm happy with this now, lets go! 💃

@rmoestl rmoestl merged commit fab0cf1 into master Apr 11, 2018
@rmoestl rmoestl deleted the fixed-size-shapes branch April 11, 2018 18:18
@etpinard etpinard mentioned this pull request Apr 11, 2018
Closed
@astrojuanlu astrojuanlu mentioned this pull request Apr 15, 2018
Closed
@astrojuanlu
Copy link

astrojuanlu commented Apr 18, 2018 •
edited
Loading

Thanks for this new feature! As a Python user, I understand that the offline mode always uses the latest release from the CDN so this should be ready to use. Have the online docs been updated accordingly?

@rmoestl rmoestl removed their assignment May 9, 2018
@rmoestl
Copy link
Contributor Author

rmoestl commented May 10, 2018

@Juanlu001 ICYMI, online docs have been updated: https://plot.ly/javascript/reference/#layout-shapes

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

@alexcjohnson alexcjohnson alexcjohnson left review comments

@etpinard etpinard Awaiting requested review from etpinard

Assignees
No one assigned
Labels
feature something new
Projects
None yet
Milestone
v1.36.0
Development

Successfully merging this pull request may close these issues.
4 participants
@rmoestl @alexcjohnson @astrojuanlu @etpinard