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

property x0shift, x1shift, y0shift, y1shift for adjusting the shape coordinates #7005

Merged
merged 26 commits into from
Jul 16, 2024

Conversation

my-tien
Copy link
Contributor

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

This property adjusts the shape coordinates if xref/yref references a (multi-)category axis.

A shape at position x0/x1 = "A"
image

with x0shift = 0.5
image

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 my-tien changed the title property shape.x_shift/y_shift for adjusting the shape coordinates property x_shift/y_shift for adjusting the shape/selection coordinates May 27, 2024
@archmoj archmoj added feature something new community community contribution status: reviewable labels May 30, 2024
'the reference unit.'
].join(' ')
},

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add these to newselection and test it.

"y1": 0.5,
"xshift": -0.25,
"yref": "paper"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this also working for a path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this should only do something when using x0, x1, y0, y1

@archmoj
Copy link
Contributor

archmoj commented May 31, 2024

Interesting PR for shapes and selections.
But I started wondering perhaps we should add a categoryshift option to Cartesian axes instead?

@archmoj
Copy link
Contributor

archmoj commented May 31, 2024

I am wondering if adding a categoryselectionstart, categoryselectionend, categoryshapestart and categoryshapeend to cartesian axes is a better option?

@archmoj
Copy link
Contributor

archmoj commented May 31, 2024

Let's ask @alexcjohnson to review 🔎

@archmoj archmoj requested review from alexcjohnson and emilykl May 31, 2024 15:11
@my-tien
Copy link
Contributor Author

my-tien commented Jun 3, 2024

I am wondering if adding a categoryselectionstart, categoryselectionend, categoryshapestart and categoryshapeend to cartesian axes is a better option?

Here is a PR with this suggestion for shapes only: #7010 (I actually only included selection in this PR, because it reuses shape properties.)

@archmoj
Copy link
Contributor

archmoj commented Jun 4, 2024

Considering #7010 (comment) discussion,
two attributes are needed for start and the end i.e. instead of xshift which is already implemented we need x0shift and x1shift.

@archmoj
Copy link
Contributor

archmoj commented Jul 11, 2024

Thanks for the simplification.
Please update the PR title and description.
Also zzz_shape_shift_vertical image test is failing now.

@archmoj
Copy link
Contributor

archmoj commented Jul 11, 2024

@stephprobst Do you have a use case for date axes?
If so it could be possible to add these options for other axis types.

@stephprobst
Copy link

@stephprobst Do you have a use case for date axes? If so it could be possible to add these options for other axis types.

@archmoj No, I don't think this is relevant for other axes. The rationale for this property is to allow exact positioning of a shape based on the axis value. For categorical axes this is needed, because it allows the combination of categorical and numerical values (e.g. country "Canada" plus a shift of 0.5 to position the shape at the end of the categories interval on the axis). On numerical or date axes this isn't really needed, since the user can directly select the precise numerical or date value (e.g. "2023-12-31" to position a shape at the end of year 2023).

My proposal would be to skip the implementation for date and numeric axes.

@my-tien my-tien changed the title property x0shift, x1shift, y0shift, y1shift for adjusting the shape/selection coordinates property x0shift, x1shift, y0shift, y1shift for adjusting the shape coordinates Jul 11, 2024
@archmoj
Copy link
Contributor

archmoj commented Jul 11, 2024

When shape.editable is set to true, clicking the shape and editing the position of a vertex result in the wrong positioning of the updated shape.

@archmoj
Copy link
Contributor

archmoj commented Jul 11, 2024

Please also test texttemplate on your mocks to show the slopes similar to those in the text_on_shapes_texttemplate mock.
Thank you!

@my-tien
Copy link
Contributor Author

my-tien commented Jul 15, 2024

Please also test texttemplate on your mocks to show the slopes similar to those in the text_on_shapes_texttemplate mock. Thank you!

I noticed that I didn't account for the shift for the texttemplate. Fixed that and added a test shape with slope and xcenter.

@my-tien
Copy link
Contributor Author

my-tien commented Jul 15, 2024

When shape.editable is set to true, clicking the shape and editing the position of a vertex result in the wrong positioning of the updated shape.

Fixed

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.

Thanks @my-tien for the PR 🥇
💃

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.

5 participants