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

Component | Axis | Add rotate label fit mode #315

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yanneves
Copy link
Contributor

@yanneves yanneves commented Nov 23, 2023

image

Adds a new option tickTextFitMode="rotate" on the Axis component. Inspired by d3fc/d3fc's d3fc-axis.

@reb-dev reb-dev self-requested a review November 27, 2023 21:34
@reb-dev reb-dev self-assigned this Nov 27, 2023
@rokotyan
Copy link
Contributor

@yanneves Thank you for this wonderful contribution and sorry for the delayed response!

That's a great idea, and we're definitely going to add that feature to the library. There are a few things we'd like to discuss before we proceed:

  1. We think it makes sense to have the rotation property separate from FitMode, since users may want labels to be trimmed/wrapped as well as rotated.
  2. We also think it makes sense to make the rotation angle configurable.

@reb-dev Would you like to add anything to it?

@yanneves
Copy link
Contributor Author

Thanks @rokotyan glad you like it!

Yeah that makes sense to me.

  1. We think it makes sense to have the rotation property separate from FitMode, since users may want labels to be trimmed/wrapped as well as rotated.

👍

  1. We also think it makes sense to make the rotation angle configurable.

That would be nice. d3fc's implementation rotates dynamically based on the width of the axis but I struggled to get all those attributes in one place to calculate. You can see the logic I used based on 30deg rotation for vertical and 60deg rotation for horizontal axes at packages/ts/src/utils/text.ts:508.

@rokotyan
Copy link
Contributor

@yanneves

d3fc's implementation rotates dynamically based on the width of the axis

Yeah, that's probably an overkill. But I think it would be nice to be able to specify a custom angle when you need it.

@reb-dev
Copy link
Collaborator

reb-dev commented Dec 14, 2023

@yanneves

The horizontal tick labels don't look quite right:
Screenshot 2023-12-13 at 4 33 51 PM
Ideally the closest part of the label should be anchored to its corresponding to tick. Otherwise, it can be confusing. See how chart becomes more difficult to read with longer labels:

Screenshot 2023-12-13 at 5 10 09 PM

@reb-dev
Copy link
Collaborator

reb-dev commented Dec 14, 2023

@yanneves cc: @rokotyan

As for keeping rotation separate from FitMode, I agree because I imagine users would often want trimming/wrapping along with rotation. I also noticed when testing your branch that labels get rotated even when they would normally fit, so it is more of a configuration property than a method for fitting text in its current state.

I propose that we add a new axis configuration property (and a property in UnovisWrappedText) for this behavior. And in the future, I think it would be great to have option to specify the rotation angle like @rokotyan mentioned.

Copy link
Collaborator

@reb-dev reb-dev left a comment

Choose a reason for hiding this comment

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

@yanneves Thank you for your contribution! This will be a great addition to the library.

I left some comments and added onto @rokotyan's feedback, feel free to reach out with any questions.

@yanneves
Copy link
Contributor Author

hey @reb-dev thanks for testing out the branch and sharing feedback! sorry it's taken me this long to follow up

the anchoring proved the most challenging, my aim was to centre the tick labels but I think having these work consistently with the vertical axis and anchor the right-side looks more correct, so tick label alignment should always be treated as TextAlign.Right

sorry to say I won't get the chance to take this further myself, but you're welcome to use my contribution as the basis for getting this implemented should you need it

@reb-dev reb-dev assigned lee00678 and unassigned reb-dev May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants