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

Add sync option to tickmode of cartesian axes to sync ticks with overlaying axis #6356

Merged
merged 60 commits into from
Jan 12, 2023

Conversation

VictorBezak
Copy link
Contributor

@VictorBezak VictorBezak commented Nov 2, 2022

The Issue

#1962

Developers:

Our Solution

This PR is an attempt to adapt the logic originating from https://github.com/VictorBezak/Plotly_Multi-Axes_Gridlines
in order to integrate it with the plotly schema. This PR is not fully tested and ready to be merged, but we wanted to get some feedback at this stage as we feel we now have a viable MVP solution.

How To Use

  • You can now set tickmode: "sync" on your 2nd yaxis that also has property overlaying set.
  • for testing, we've added a new mock called new_tickmode_sync.json

Things Tested and Working

  • positive/negative ranges
  • zoom
  • click & drag y-axis up/down

Not Done Yet

  • we haven't written any tests yet!

@VictorBezak VictorBezak changed the title First poc tickmode sync Ability to sync the dtick values of multiple y-axes with new tickmode value "sync" Nov 2, 2022
@VictorBezak
Copy link
Contributor Author

VictorBezak commented Nov 2, 2022

@alexcjohnson @jackparmer @archmoj
I've been receiving help for the last 5 weeks from fellow developer, Filipe Santiago @filipesantiagoAM, and together we've managed to put together an MVP solution!

@fsantiago99
Copy link
Contributor

@archmoj @alexcjohnson Could you have a look here and give us some feedback please?

@archmoj archmoj added feature something new community community contribution status: reviewable labels Nov 11, 2022
@archmoj
Copy link
Contributor

archmoj commented Nov 11, 2022

Thanks very much for the PR.
The image test is broken now.
Please rename new_tickmode_sync.json to z-new_tickmode_sync.json.
Then download the baseline here and place it at test/image/baselines/z-new_tickmode_sync.png.
Thank you!

@archmoj
Copy link
Contributor

archmoj commented Nov 11, 2022

This is pretty cool. When I drag the right axis the grid lines stay at their initial locations and only the tick labels are updated.
Wondering if you could possibly make that for the other axis i.e. when dragging the left axis the grid lines stay there?

@filipesantiagoAM
Copy link
Contributor

All good with the checks here.

Wondering if you could possibly make that for the other axis i.e. when dragging the left axis the grid lines stay there?

@archmoj I was trying to put this to work but looks a bit tricky because when we drag one axis, this should be the only one who refreshes the ticks. If I automatically refresh the others should be a bit mess, especially when we have more than two axes and some are connected and some not, if it makes sense. Of course I'm open to hear your tips and tricks to approach this feature. Thanks

@filipesantiagoAM
Copy link
Contributor

@archmoj I know this PR requires more effort to be merged but if you could have a look and give us some feedback would be great. This is the most desired feature for me in plotly.js.

Thanks

test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved

// get the tick for the current axis based on position
var vali = ax.p2l(pos);
var val1 = ax.p2l(pos - 0.1);
Copy link
Contributor

@filipesantiagoAM filipesantiagoAM Jan 4, 2023

Choose a reason for hiding this comment

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

@archmoj Now this code makes sense? Now I feel we are at the same position we were before when you suggest this piece of code.

            var vali = ax.p2l(pos);
            var val1 = ax.p2l(pos - 0.5);
            var val2 = ax.p2l(pos + 0.5);
            var d = 1 + Math.round(Math.log10(Math.abs(val2 - val1)));
            var e = Math.pow(10, -d);
            var valR = Math.round(vali * e) / e;
            var objR = axes.tickText(ax, valR);
            var obj = axes.tickText(ax, vali);
            obj.text = objR.text;
Suggested change
var val1 = ax.p2l(pos - 0.1);
var vali = ax.p2l(pos);
var obj = axes.tickText(ax, vali);

I agree we need to improve the roundings but not sure how to do it.

@archmoj
Copy link
Contributor

archmoj commented Jan 4, 2023

@filipesantiagoAM at this point the logic and behavior looks good to me.
Let's improve our tests a bit then it should be ready for final review.
Thank you!

@@ -28,6 +28,8 @@ module.exports = function handleTickValueDefaults(containerIn, containerOut, coe

if(tickmode === 'auto') {
coerce(prefix + 'nticks');
} else if(tickmode === 'sync') {
coerce('overlaying');
Copy link
Contributor

Choose a reason for hiding this comment

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

@alexcjohnson what do you suggest here?

It looks like we shouldn't use overlaying attribute "directly":

// overlaying: not used directly, just put here for reference
// values are false and any other same-letter axis id that's not
// itself overlaying anything
overlaying: {
valType: 'enumerated',
values: [
'free',
constants.idRegex.x.toString(),
constants.idRegex.y.toString()
],
editType: 'plot',
description: [
'If set a same-letter axis id, this axis is overlaid on top of',
'the corresponding same-letter axis, with traces and axes visible for both',
'axes.',
'If *false*, this axis does not overlay any same-letter axes.',
'In this case, for axes with overlapping domains only the highest-numbered',
'axis will be visible.'
].join(' ')
},

Instead overlaying is coerced here:

overlaying = Lib.coerce(containerIn, containerOut, {
overlaying: {
valType: 'enumerated',
values: [false].concat(overlayableAxes),
dflt: false
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm understanding this correctly, the situation we want to avoid is tickmode:'sync' with no overlaying. And the challenge is that when we get here, overlaying hasn't been coerced yet. But @archmoj is right, we can't use the attribute directly and we don't have enough information at this point to coerce it correctly anyway, since we don't know what other axes exist yet.

So what I'd suggest is to leave that block out here, but fix it when we DO have enough information to do that - right after that spot @archmoj pointed to where we coerce overlaying, there's this block:

If we get in there with tickmode === 'sync', we should force it to 'auto', which means we also need nticks:

if(containerOut.tickmode === 'sync') {
    containerOut.tickmode = 'auto';
    coerce('nticks');
}

"editType": "ticks",
"impliedEdits": {},
"valType": "enumerated",
"values": [
"auto",
"linear",
"array"
"array",
"sync"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps sync shouldn't be added to minor axes.
If major ticks are sync, then auto option for minor ticks on that axis should copy minor ticks from the base axis. No?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, removing.

@alexcjohnson
Copy link
Collaborator

@VictorBezak @filipesantiagoAM just playing with this now, it's sooo satisfying to pan/zoom sync axes ☺️ Very nicely done!

@filipesantiagoAM
Copy link
Contributor

@alexcjohnson @archmoj All good I believe

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Looks great! It'll need a draftlog entry, but aside from that I think we're good to go! 💃

@archmoj mentioned to me that the original suggestion I had (over 5 years ago! 🤯) for the new attribute value was 'match overlay', and that this perhaps does a better job of self-documenting "oh, to use this I also need to set the overlaying attribute," also that 'sync' could imply a two-way interaction whereas this is one-way: the main axis sets the tick positions, all overlaying axes just use those.

While I think those arguments make sense, I also see the benefit of 'sync': It's shorter and easier to remember (vs 'match overlay', or is it 'matches overlaying' or ... ?) and still, to my eye anyway, fairly intuitive and well-documented by the description. So I'm happy to leave it as is.

@archmoj archmoj changed the title Ability to sync the dtick values of multiple y-axes with new tickmode value "sync" Add 'sync' option to tickmode of cartesian axes to sync ticks with overlaying axis Jan 12, 2023
@archmoj archmoj changed the title Add 'sync' option to tickmode of cartesian axes to sync ticks with overlaying axis Add sync option to tickmode of cartesian axes to sync ticks with overlaying axis Jan 12, 2023
@archmoj archmoj merged commit 05d6de2 into plotly:master Jan 12, 2023
@hottwaj
Copy link

hottwaj commented Jan 27, 2023

Hi all, I was very glad to see that this longstanding issue was worked through and solved - thanks very much for your work on it!

I have tried out the new tickmode='sync' option in a plot with plotly.js release 2.18.0 , but I get "non-rounded" tick values on the y2 axis e.g. 169.4, 145.2 etc

See example here & screenshot at end of this post
https://codepen.io/hottwaj/pen/MWBQqra

Am I missing something, or is that the intended behaviour at the moment?

Thanks!

image

@alexcjohnson
Copy link
Collaborator

@hottwaj that’s correct, making the ticks also round numbers for the synced axis would require adjusting its (auto)range, which was out of scope for this PR. I do like that idea, though I don’t think we want I to to be the default. But we’d gladly accept a PR adding an option to expand the autorange until the tick values are round numbers. With the caveat that in some cases this will not be possible, like syncing axes of different types.

@hottwaj
Copy link

hottwaj commented Jan 27, 2023

OK got it, thanks for clarifying - I misunderstood what was being covered here. Great work and thanks for getting this implemented :)

@moisesosorio
Copy link

thanks @VictorBezak !, it is helping so much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants