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

Fixed size shapes #2532

Merged
merged 8 commits into from Apr 11, 2018

Conversation

Projects
None yet
4 participants
@rmoestl
Copy link
Contributor

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 some commits Mar 20, 2018

Move assertions from hover_label_test.js to custom_assertions.js #2193
- Reason: prepare writing fixed size shape tests.
Introduce fixed size shapes #2193
- 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 Apr 6, 2018

xsizemode: {
valType: 'enumerated',
values: ['data', 'pixel'],
dflt: 'data',

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 6, 2018

Contributor

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

This comment has been minimized.

Copy link
@rmoestl

rmoestl Apr 10, 2018

Author Contributor

Good point. I'll change that.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

@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 some commits Apr 10, 2018

Add an image test for fixed size shapes #2193
- 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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

"config": {
"editable": "true"
}
}

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 10, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

I'll check out the cursor position thing.

@rmoestl

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

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.

Fix drag cursor bug for shapes
- 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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

Wow, good catch!!

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

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

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image 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 fixed-size-shapes branch Apr 11, 2018

@etpinard etpinard referenced this pull request Apr 11, 2018

Closed

Fixed size circle shape #2193

@Juanlu001

This comment has been minimized.

Copy link

commented Apr 18, 2018

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

This comment has been minimized.

Copy link
Contributor Author

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
You can’t perform that action at this time.