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

Revert "Improvements to how the X Axis is rendered" #65

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

vicanso
Copy link
Owner

@vicanso vicanso commented Dec 27, 2023

Reverts #56

@vicanso vicanso merged commit c302d0f into main Dec 27, 2023
@jentfoo
Copy link
Contributor

jentfoo commented Jan 2, 2024

@vicanso Can you expand on why this change was reverted? Reverting this change resulted in a breaking change on our project.

@vicanso
Copy link
Owner Author

vicanso commented Jan 2, 2024

I am sorry, the situation is mentioned at #63, the label should be centered. I found this change set the last label like this:

if index == len(opt.TextList) - 1 {
    x = start - box.Width() + 10
} else {
    x = start - box.Width()>>1
}

@jentfoo
Copy link
Contributor

jentfoo commented Jan 3, 2024

The changes in my PR changed it so that the label positions would be consistent and span across the entire X axis length. For that reason the last label position needed to be adjusted (if it was kept centered it would be cut off by the edge of the image).

Unfortunately I can't read #63 to understand the nuance. But if possible I would recommend re-introducing my changes and find a more appropriate option for positioning the last label.

To reiterate the most significant value that is lost by reverting #56; By not having a tick at the end of the X axis, there is no way to know the span for the data. #56 ensured that the tick is always at the start and end of the graph so the range of the data is well understood. This is a critical issue, and will result in me needing to fork the library if the changes are not reintroduced.

@vicanso
Copy link
Owner Author

vicanso commented Jan 3, 2024

I know your purpose, I will find a way for positioning the last label.

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.

2 participants