plotly.js icon indicating copy to clipboard operation
plotly.js copied to clipboard

Add `ticklabelstandoff` and `ticklabelrunoff` to cartesian axes

Open my-tien opened this issue 1 year ago • 15 comments
trafficstars

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.

my-tien avatar May 27 '24 15:05 my-tien

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 avatar May 28 '24 11:05 my-tien

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 :)

my-tien avatar May 28 '24 11:05 my-tien

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

my-tien avatar Jun 10 '24 10:06 my-tien

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!

archmoj avatar Jun 10 '24 16:06 archmoj

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

my-tien avatar Jun 12 '24 06:06 my-tien

Looking good to me. I ask @alexcjohnson and @LiamConnors for their reviews as well as approving the attribute names and descriptions. Thanks.

archmoj avatar Jun 12 '24 11:06 archmoj

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

It's no longer the mapbox test which is failing. The image test I rerun from start and it passed. But please have a look at this test to see we have to pull @flaky tag to it after these changes? https://github.com/plotly/plotly.js/blob/caf32e57cfdbd370076b926f84c114093f62f0b9/test/jasmine/tests/polar_test.js#L450

archmoj avatar Jun 12 '24 13:06 archmoj

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

It's no longer the mapbox test which is failing. The image test I rerun from start and it passed. But please have a look at this test to see we have to pull @flaky tag to it after these changes?

https://github.com/plotly/plotly.js/blob/caf32e57cfdbd370076b926f84c114093f62f0b9/test/jasmine/tests/polar_test.js#L450

Thanks @archmoj when I run the test locally, it doesn't fail, so I think it is indeed flaky.

my-tien avatar Jun 13 '24 12:06 my-tien

it('should be able to restyle radial axis title', function(done) {

Thanks for testing out. Please add @flaky tag to the start of test description like this:

 it('@flaky should be able to restyle radial axis title', function(done) { 

archmoj avatar Jun 13 '24 13:06 archmoj

I think this could also help resolve #1673. :tada: which is one the oldest open issues now. Please add Resolves #1673 to the PR description. Thank you!

archmoj avatar Jun 17 '24 13:06 archmoj

Please revert 6ea832f then fetch upstream/master and merge it into this branch so that the CI tests pass. cc: #7030.

archmoj avatar Jun 18 '24 19:06 archmoj

I like the attribute names. But @LiamConnors what do you think of new attribute names here? cc: @emilykl

archmoj avatar Jun 25 '24 14:06 archmoj

@emilykl could you please have a look at this one?

gvwilson avatar Jun 26 '24 12:06 gvwilson

I wonder if it would be more intuitive to specify this as something like ticklabeloffset: {x: 20, y: 10} ie a number of pixels to move the labels in the x direction (positive to the right, negative to the left) and the y direction (positive up, negative down)?

standoff is clear to me that it refers to moving orthogonal to the axis and away from the line or tick marks. But runoff I find confusing, and I can't think of another name that clearly refers to motion in that direction, which is what led me to suggest changing both of them.

alexcjohnson avatar Jun 26 '24 14:06 alexcjohnson

I wonder if it would be more intuitive to specify this as something like ticklabeloffset: {x: 20, y: 10} ie a number of pixels to move the labels in the x direction (positive to the right, negative to the left) and the y direction (positive up, negative down)?

standoff is clear to me that it refers to moving orthogonal to the axis and away from the line or tick marks. But runoff I find confusing, and I can't think of another name that clearly refers to motion in that direction, which is what led me to suggest changing both of them.

@alexcjohnson Thanks very much for the feedback. At the start of this PR x and y attributes are used. But I found them quite confusing to work with. Here it was suggested to use standoff.

For the second attribute (runoff) name let's see if @emilykl and @LiamConnors would have any suggestion?

archmoj avatar Jun 26 '24 15:06 archmoj

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

archmoj avatar Jul 02 '24 15:07 archmoj

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

archmoj avatar Jul 04 '24 13:07 archmoj

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

stephprobst avatar Jul 04 '24 13:07 stephprobst

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

archmoj avatar Jul 05 '24 13:07 archmoj