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

Conversation

ayjayt
Copy link
Contributor

@ayjayt ayjayt commented Dec 27, 2023

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 a ticksOut 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.

@ayjayt ayjayt mentioned this pull request Dec 27, 2023
12 tasks
@archmoj
Copy link
Contributor

archmoj commented Jan 2, 2024

Thanks very much for the PR.
Please add a draft log as well as a jasmine test; and this should be ready to go.

@archmoj archmoj added bug something broken community community contribution labels Jan 2, 2024
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.

// 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.

@@ -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.

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?

@ayjayt
Copy link
Contributor Author

ayjayt commented Jan 3, 2024

So, this should be about done. If I use Alex's strategy for implementing domain array/full domain, this bug won't appear in that feature.

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

@archmoj
Copy link
Contributor

archmoj commented Jan 4, 2024

Thanks for all the effort on this pull.
We are open to move to ES6 syntax in our code base.
We've already managed to make that move for the dependencies; but not for our files in src and test folders at the moment.
So please feel free to submit a pull request to make that happen.
Alternatively you may possibly help the exciting pull #6680 which needs a similar configuration.

Thanks again.
🥇 🏆 💃

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

Successfully merging this pull request may close these issues.

2 participants