-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 switch to specify major/minor in axes.arrayTicks()
#6829
Add switch to specify major/minor in axes.arrayTicks()
#6829
Conversation
Thanks very much for the PR. |
test/jasmine/tests/axes_test.js
Outdated
function generateTickConfig(){ | ||
standardConfig = {tickmode: 'array', ticks: 'inside', ticklen: 1, showticklabels: false}; | ||
|
||
// Number of ticks will be random |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use Lib.pseudoRandom
as well as Lib.seedPseudoRandom
as it applied by other jasmine tests.
test/jasmine/tests/axes_test.js
Outdated
// test the nearest exported functions (calcTicks) | ||
describe('arrayTicks', function() { | ||
for(let i = 0; i < 4; i++) { | ||
(function(i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please declare this function outside the loop and use ES5 to pass the syntax test.
src/plots/cartesian/axes.js
Outdated
@@ -1261,7 +1261,8 @@ function syncTicks(ax) { | |||
return ticksOut; | |||
} | |||
|
|||
function arrayTicks(ax) { | |||
function arrayTicks(ax, major) { | |||
if (major === undefined) throw new Error("arrayTicks must specify ticktype") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this line?
After you added it in 9a6de43, some tests failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good actually!
I put that line in there to force failure if other calls relied on default behavior.
Jasmine is wonky on my local box so I didn't catch those failed tests before push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arrayTicks will now behave in a legacy manner if the caller isn't aware to use the major/minor switch.
test/jasmine/tests/axes_test.js
Outdated
r2l: Lib.cleanNumber, | ||
d2l: Lib.cleanNumber, | ||
_separators: ',', | ||
range: [0, 1000], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of mocking the xaxis
, can't we simply call Plotly.newPlot(...)
and then inspect the gd._fullLayout.xaxis
?
So, this should be about done. If I use Alex's strategy for implementing That said, I'm not too thrilled with Jasmine nor ES5, so I have started a small refactor fork: https://github.com/geopozo/plotly.js/tree/master?tab=readme-ov-file#refactor |
Thanks for all the effort on this pull. Thanks again. |
Long explanation in: #6828: but basically
calcTicks()
tries and thinks its differentiating between major and minor ticks when doing some adjustments, but it is not actually. It ends up creating aticksOut
that contains each tick duplicated once instead.Actual changes simple. If this function is used outside of
axes.js
, should probably make default behavior same as original behavior.I will do the draft log tomorrow.
Fixing this hidden error would solve all problems in my other draft pull request and allow
proportional
tickmode for minor ticks.