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 switch to specify major/minor in axes.arrayTicks() #6829

Merged
merged 11 commits into from
Jan 4, 2024
1 change: 1 addition & 0 deletions draftlogs/6829_fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Add argument to `arrayTicks()` to render major OR minor [[#6829](https://github.com/plotly/plotly.js/pull/6829)]
10 changes: 6 additions & 4 deletions src/plots/cartesian/axes.js
Original file line number Diff line number Diff line change
Expand Up @@ -949,10 +949,10 @@ axes.calcTicks = function calcTicks(ax, opts) {
if(mockAx.tickmode === 'array') {
if(major) {
tickVals = [];
ticksOut = arrayTicks(ax);
ticksOut = arrayTicks(ax, major);
} else {
minorTickVals = [];
minorTicks = arrayTicks(ax);
minorTicks = arrayTicks(ax, false);
}
continue;
}
Expand Down Expand Up @@ -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")
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

var rng = Lib.simpleMap(ax.range, ax.r2l);
var exRng = expandRange(rng);
var tickMin = Math.min(exRng[0], exRng[1]);
Expand All @@ -1279,7 +1280,8 @@ function arrayTicks(ax) {

var ticksOut = [];
for(var isMinor = 0; isMinor <= 1; isMinor++) {
if(isMinor && !ax.minor) continue;
if(!isMinor && !major) continue;
if(isMinor && (!ax.minor || major)) continue;
var vals = !isMinor ? ax.tickvals : ax.minor.tickvals;
var text = !isMinor ? ax.ticktext : [];

Expand Down
63 changes: 62 additions & 1 deletion test/jasmine/tests/axes_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ var supplyDefaults = require('../assets/supply_defaults');

describe('Test axes', function() {
'use strict';

describe('swap', function() {
it('should swap most attributes and fix placeholder titles', function() {
var gd = {
Expand Down Expand Up @@ -8217,3 +8216,65 @@ describe('shift tests', function() {
});
});
});
describe('test tickmode calculator', function() {
beforeEach(function() {
gd = createGraphDiv();
});

afterEach(destroyGraphDiv);

function generateTickConfig(){
standardConfig = {tickmode: 'array', ticks: 'inside', ticklen: 1, showticklabels: false};

// Number of ticks will be random
Copy link
Contributor

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.

var n = Math.floor(Math.random() * 99) + 1;
tickVals = [];
for(let i = 0; i <= n; i++) tickVals.push(i);
standardConfig['tickvals'] = tickVals;
standardConfig['ticktext'] = tickVals;
return standardConfig;
}
var ticksOff = {tickmode:"array", ticks: '', tickvals:[], ticktext:[], ticklen: 0, showticklabels: false};
// the goal is target the arrayTicks() function, the subject of PR: https://github.com/plotly/plotly.js/pull/6829
// we test xMajor and xMinor in on/on on/off off/on and off/off
// since we can't unit test functions that are not exported, we shim functions we don't care about instead and
// test the nearest exported functions (calcTicks)
describe('arrayTicks', function() {
for(let i = 0; i < 4; i++) {
(function(i) {
Copy link
Contributor

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.

it('should return the specified correct number of major ticks and minor ticks', function() {
const BOTH = 0;
const MAJOR = 1;
const MINOR = 2;
const NEITHER = 3;
var xMajorConfig = ticksOff;
var xMinorConfig = ticksOff;
if(i == BOTH) {
xMajorConfig = generateTickConfig();
xMinorConfig = generateTickConfig();
} else if(i == MAJOR) {
xMajorConfig = generateTickConfig();
} else if(i==MINOR) {
xMinorConfig = generateTickConfig();
} else if(i == NEITHER) {
// Do nothing, all ticks off
}
xaxis = {
r2l: Lib.cleanNumber,
d2l: Lib.cleanNumber,
_separators: ',',
range: [0, 1000],
Copy link
Contributor

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?

...xMajorConfig,
minor: {
...xMinorConfig,
},
};
Axes.prepMinorTicks = function() { return }; // Not part of this test
Axes.prepTicks = function() { return }; // Not part of this test
ticksOut = Axes.calcTicks(xaxis);
expect(ticksOut.length).toEqual(xMajorConfig.tickvals.length + xMinorConfig.tickvals.length);
});
})(i);
}
});
});