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

Bug/pikul standoff position #6889

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

ayjayt
Copy link
Contributor

@ayjayt ayjayt commented Feb 9, 2024

Fixes #6890

Breakdown of the fix:

ax._depth is not calculated properly in the standoff conditional:

} else if(ax.title.hasOwnProperty('standoff')) {
seq.push(function() {
ax._depth = majorTickSigns[4] * (getLabelLevelBbox()[ax.side] - mainLinePositionShift);
});
}

Lots of positioning calculations rely on that getLabelLevelBbox() which, bypassing the cache, is this:

function calcLabelLevelBbox(ax, cls) {
    var top, bottom;
    var left, right;

    if(ax._selections[cls].size()) {
        ... // iterate over ticklabels and get their DOM properties
    } else {
        top = bottom = left = right = 0;
    }

    return { top: top, bottom: bottom, left: left, right: right, height: bottom - top, width: right - left };
}

The issue being the else branch, where we have no ticklabels on which to loop. We shouldn't return 0 as null- 0 is a legitimate coordinate and will fool consumers. Instead, if we accept that an empty bounding box still has a position, but with a width and height of 0, we can calculate said position (where the ticklabel would be) and return a bounding box with no width or height. That's what I do in the fix using makeLabelFns(), and it seems to work well.

var dummyCalc = axes.makeLabelFns(ax, mainLinePositionShift);
top = bottom = dummyCalc.yFn({dx: 0, dy: 0, fontSize: 0}); // I don't know what dx, dy is
left = right = dummyCalc.xFn({dx: 0, dy: 0, fontSize: 0});

Let's see if these initial commits past the tests. Probably get rid of the mocks?

@archmoj archmoj added bug something broken community community contribution status: reviewable labels Mar 6, 2024
@archmoj
Copy link
Contributor

archmoj commented Mar 6, 2024

Thanks for the PR. It seems I don't have permission to push to your repository.
So I merge this and add the tests in a separate PR.
💃

@archmoj archmoj merged commit 5b1e104 into plotly:master Mar 6, 2024
1 check passed
@ayjayt
Copy link
Contributor Author

ayjayt commented Mar 6, 2024

Thanks @archmoj , sorry if I'm forgetting the protocol a bit with PRs here. I'm a bit swamped too but I'll get back to these probably by the end of the week to do merges/changelogs/tests/todo lists etc.

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.

BUG: standoff causes axis title position to break if no tick labels
2 participants