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 ticklabelstandoff and ticklabelshift to cartesian axes #7006

Merged
merged 26 commits into from
Jul 5, 2024

Conversation

my-tien
Copy link
Contributor

@my-tien my-tien commented May 27, 2024

These properties shift the axis tick labels in parallel or orthogonally to the axis (in pixels).

Resolves #1673

Disclaimer I am required to add that…

the software is provided "as is", without warranty of any kind, express or implied, including but not limited to the warranties of merchantability, fitness for a particular purpose and noninfringement. in no event shall the authors or copyright holders be liable for any claim, damages or other liability, whether in an action of contract, tort or otherwise, arising from, out of or in connection with the software or the use or other dealings in the software.

Also modifies mock date_axes_period2 to test the new properties
@my-tien
Copy link
Contributor Author

my-tien commented May 28, 2024

Help appreciated for these failing bundle tests (I failed to get the debugger running for this).
Is "ticks" not the correct editType for my new properties?

Firefox 126.0 (Windows 10) plot schema has valid `editType` in all attributes and containers FAILED
        Expected false to be true, 'carpet.aaxis.ticklabelshiftx: "ticks"'.
        <Jasmine>
        @webpack://plotly.js/./test/jasmine/bundle_tests/plotschema_test.js?:185:62
        exports.crawl/<@webpack://plotly.js/./src/plot_api/plot_schema.js?:102:13
        exports.crawl@webpack://plotly.js/./src/plot_api/plot_schema.js?:98:22
        exports.crawl/<@webpack://plotly.js/./src/plot_api/plot_schema.js?:105:15
        exports.crawl@webpack://plotly.js/./src/plot_api/plot_schema.js?:98:22
        assertTraceSchema/<@webpack://plotly.js/./test/jasmine/bundle_tests/plotschema_test.js?:27:25
        assertTraceSchema@webpack://plotly.js/./test/jasmine/bundle_tests/plotschema_test.js?:26:25
        @webpack://plotly.js/./test/jasmine/bundle_tests/plotschema_test.js?:183:22
        <Jasmine>

@my-tien
Copy link
Contributor Author

my-tien commented May 28, 2024

Help appreciated for these failing bundle tests (I failed to get the debugger running for this). Is "ticks" not the correct editType for my new properties?

Firefox 126.0 (Windows 10) plot schema has valid `editType` in all attributes and containers FAILED
        Expected false to be true, 'carpet.aaxis.ticklabelshiftx: "ticks"'.
        <Jasmine>
        @webpack://plotly.js/./test/jasmine/bundle_tests/plotschema_test.js?:185:62
        exports.crawl/<@webpack://plotly.js/./src/plot_api/plot_schema.js?:102:13
        exports.crawl@webpack://plotly.js/./src/plot_api/plot_schema.js?:98:22
        exports.crawl/<@webpack://plotly.js/./src/plot_api/plot_schema.js?:105:15
        exports.crawl@webpack://plotly.js/./src/plot_api/plot_schema.js?:98:22
        assertTraceSchema/<@webpack://plotly.js/./test/jasmine/bundle_tests/plotschema_test.js?:27:25
        assertTraceSchema@webpack://plotly.js/./test/jasmine/bundle_tests/plotschema_test.js?:26:25
        @webpack://plotly.js/./test/jasmine/bundle_tests/plotschema_test.js?:183:22
        <Jasmine>

Nvm, I figured it out, print debugging ftw :)

…ks → calc

carpet is a trace and therefore doesn't support editType ticks
@archmoj archmoj added feature something new community community contribution status: reviewable labels May 28, 2024
@@ -698,6 +698,22 @@ module.exports = {
'In other cases the default is *hide past div*.'
].join(' ')
},
ticklabelshiftx: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering where we should put the x or y in the attribute names?
In #7005 we have xshift, here we have shiftx.

Copy link
Contributor Author

@my-tien my-tien May 31, 2024

Choose a reason for hiding this comment

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

Hm, true. in #7005 my reasoning was that in the shape properties we have x0, x1 and xref. So I added xshift.

Here I had a similar reasoning, there were already properties starting with ticklabel*. And since in ticklabelxshift it is maybe hard to make out the 'x', I moved it to the end…

No strong opinion here.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHo we shouldn't use x and y in these attribute names.
Instead something like standoff and runoff could be used so that when switching the orientation e.g. on a colorbar from horizontal to vertical everything works without a need to adjusting these parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to ticklabelrunoff (shifts in parallel to axis) and ticklabelstandoff (shifts orthogonally to axis)

test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
my-tien added 6 commits June 3, 2024 17:07
And remove this feature from all non-cartesian traces. If other plots should be supported, this can be added and tested in separate PRs.
…axis.side and ticklabelposition.

Standoff moves labels farther outside for outside labels and farther inside for inside labels.

Runoff moves labels further along the axis range.
@my-tien
Copy link
Contributor Author

my-tien commented Jun 10, 2024

Failing mapbox test appears to be unrelated to my changes. It looks good locally.

@archmoj
Copy link
Contributor

archmoj commented Jun 10, 2024

Failing mapbox test appears to be unrelated to my changes. It looks good locally.

To fix it, please fetch upstream/master and merge it into this branch.
Thank you!

@my-tien
Copy link
Contributor Author

my-tien commented Jun 12, 2024

The mapbox PR was already merged before, see here: 4ccaf73

draftlogs/7006_add.md Outdated Show resolved Hide resolved
@archmoj archmoj changed the title Add ticklabelstandoff and ticklabelrunoff to cartesian axes Add ticklabelstandoff and ticklabelshift to cartesian axes Jul 2, 2024
@archmoj
Copy link
Contributor

archmoj commented Jul 2, 2024

I checked with @LiamConnors and @emilykl and they both suggested to use ticklabelshift instead of ticklabelrunoff.

@gvwilson gvwilson assigned archmoj and unassigned gvwilson and emilykl Jul 3, 2024
draftlogs/7006_add.md Outdated Show resolved Hide resolved
src/plots/cartesian/axes.js Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Jul 4, 2024

@my-tien This PR is looking very good.
@stephprobst Do you want any specific figure to be tested in this PR?

@stephprobst
Copy link

@archmoj : Thanks for the mention. No, no particular figure in mind. It's a very general use case.

my-tien and others added 2 commits July 5, 2024 07:01
Update draftlog for renaming of `ticklabelrunoff` to `ticklabelshift`

Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
(no need to test if ax.side matches axis, because this check is done before)
draftlogs/7006_add.md Outdated Show resolved Hide resolved
draftlogs/7006_add.md Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Jul 5, 2024

@my-tien Just few fixes needed by you here (see my recent comments) and we should be good to merge it today!

my-tien and others added 2 commits July 5, 2024 15:02
Improve draftlog text for ticklabelstandoff and ticklabelshift

Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
…d move outside and outside ticks could move inside
Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

💃

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.

More control over tick text alignment
6 participants