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

Trying matching and scaling #5196

Closed
wants to merge 6 commits into from
Closed

Trying matching and scaling #5196

wants to merge 6 commits into from

Conversation

nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Oct 6, 2020

Resolves #3539.

@plotly/plotly_js

@nicolaskruchten
Copy link
Member Author

nicolaskruchten commented Oct 6, 2020

Trying here: https://codepen.io/nicolaskruchten/pen/KKMPJKR ... no luck yet looks like it "just works"!

Now need to figure out a way to reinstate the exclusions in the pathological cases only.

@nicolaskruchten
Copy link
Member Author

@emmanuelle check out the codepen above as a proof of concept

@@ -57,10 +57,7 @@ exports.handleConstraintDefaults = function(containerIn, containerOut, coerce, o
// 'matches' wins over 'scaleanchor' (for now)
var scaleanchor, scaleOpts;

if(!matches &&
Copy link
Member Author

Choose a reason for hiding this comment

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

@alexcjohnson so I loosened this check, but I also had to remove the check below ... is this a belt-and-suspenders situation? If I restore the check below such that we only remove scale if it's the same letter as matches, does that seem sufficient?

@archmoj archmoj added this to the v1.58.0 milestone Oct 26, 2020
@archmoj
Copy link
Contributor

archmoj commented Oct 26, 2020

DEMO: before vs after.

@alexcjohnson
Copy link
Collaborator

Thanks for the demo @archmoj

AFAICT this generally works OK for constrain: 'domain' if you specify every possible constraint you can - which is generally quite overconstrained so it's clearly going to be easy to specify impossible constraints. It's also clearly broken for constrain: 'range' and strange things happen if you try to remove some of the redundant constraints.

fixedrange currently seems to have no effect on the ranges we determine at render time, but if users zoom or pan afterward the constraints are sometimes violated as the fixedrange axis does not change its range, and sometimes this axis doesn't change while the mouse is down but it does after the mouse is released. The constraint violation doesn't bother me except that if you tried to take the resulting figure and recreate the plot in a new container it would want to change something to satisfy the constraint.

Having played with this a bit and tried to figure out as a user how I'd construct a plot like this, I really think we should keep the original condition that setting matches precludes scaleanchor and scaleratio, ie there's only one constraint defined per axis, and just allow mixed matches and scaleanchor constraints. Then to make a plot with many matching aspect-ratio-constrained subplots, you'd set a single scaleanchor constraint (eg yaxis.scaleanchor = 'x') and many matches constraints (xaxisN.matches = 'x', yaxisN.matches = 'y'). For this to mean that the aspect ratios of all of these subplots are identical, the user needs to ensure that the matches axes all have the same size domain; But to me that feels like it would be intuitive to users (and mostly hidden by px anyway 😄 )

One important observation from my testing just now: if you have a domain-constrained axis, and others match it, we need to set them all to have the same fractional domain reduction - ie matches implies not just the same range, but also inheriting the constrain attribute (I suppose constraintoward could be different though) and during rendering reducing the range of one of these axes reduces the domain of all of them.

@alexcjohnson
Copy link
Collaborator

Superseded by #5287

@alexcjohnson alexcjohnson deleted the match_and_scale branch November 20, 2020 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement scaleanchor domain constraints on matching axes
3 participants