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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement alignmentgroup and offsetgroup #3529

Merged
merged 5 commits into from Feb 19, 2019

Conversation

Projects
None yet
5 participants
@etpinard
Copy link
Member

commented Feb 8, 2019

resolves #3402 - at least the part listed in #3402 (comment)

In brief, this PR add two attributes alignmentgroup and offsetgroup to bar, histogram, box and violin traces. These attributes allow to have more control over how the cross-trace offset and width values are computed. Examining the bar-alignment-offset mock is probably the best way to grasp what these new attributes do.

cc @plotly/plotly_js and @nicolaskruchten - it would be nice if you could take a 馃憖 at the new mocks, thanks!

some TODOs:

  • Incorporate alignmentgroup and offsetgroup on matching axes, once #3506 is merged
  • Decide what alignmentgroup and offsetgroup should do across trace types
// TODO make this work across matching axes too?!?
// TODO should this work per trace-type?
// one set for bar/histogram another for box/violin?
// or just one set for all trace trace types?

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 8, 2019

Author Member

Does anyone here have a strong opinion about this?

Should alignmentgroup and offsetgroup work across trace types? Or should we match bar and histogram and box and violin groupings separately?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Feb 8, 2019

Contributor

Seems like making it work across trace types would be the most flexible - you can always just give different values to different trace types, which I guess is what the default would be. It's also probably what I'd expect as a user. It's an awfully weird use case, so not worth a huge effort, but if we omit it now and wanted to add it later it would be a breaking change.

This comment has been minimized.

Copy link
@nicolaskruchten

nicolaskruchten Feb 8, 2019

Member

Bars above for counts (say) and boxes below for distributions? Seems not that weird to me :) kinda useful actually!

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Feb 8, 2019

Contributor

Ah yeah good point. That would magically work if you have the same traces in each case, and in the same order... but break those assumptions (which could happen for good reasons) and you're back to manual alignment if we don't implement this cross-type.

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 18, 2019

Author Member

Done in e746b08

etpinard added some commits Feb 18, 2019

consider alignment and offset group across matching axes
... as well as across trace types (confirmed behavior)

to do so:
- add getAxisGroup axis_ids.js util
- mv alignmentgroup and offsetgroup coerce calls to crossTraceDefaults
@etpinard

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@plotly/plotly_js this PR is now reviewable. We'll also need this one for 1.45.0.

@archmoj could you review this one?

@archmoj archmoj assigned archmoj and unassigned archmoj Feb 18, 2019

@archmoj archmoj self-requested a review Feb 18, 2019

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Feb 19, 2019

Excellent PR 馃拑
@etpinard would you mind adding a codepen example to the PR description which may be helpful for the Q&A? @VeraZab

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

All the new mocks in a codepen:

https://codepen.io/etpinard/pen/mvoJoz

@etpinard etpinard merged commit 399d03b into master Feb 19, 2019

8 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish 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
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@etpinard etpinard deleted the offset-alignment-groups branch Feb 19, 2019

@arlowhite

This comment has been minimized.

Copy link

commented Mar 20, 2019

@etpinard thank you for this feature! One minor issue is that traces do not re-layout when toggled (reproduced in your "mocks"). So the user cannot turn-off traces using the legend to view the remaining traces at a larger size.

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

@arlowhite thanks, would you mind filing a bug report at https://github.com/plotly/plotly.js/issues/new ?

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