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 new align 'inner' for X axis #10106

Merged
merged 8 commits into from Feb 12, 2022
Merged

Add new align 'inner' for X axis #10106

merged 8 commits into from Feb 12, 2022

Conversation

Talla2XLC
Copy link
Contributor

@Talla2XLC Talla2XLC commented Jan 27, 2022

New align type: 'left-right' for options.scales['x'].ticks namespace will allow users to aling horizontal axis ticks: 'start" for first (left) tick and 'end' for last (right) tick.
Before:
image

After:
image

new align 'left-right' for options.scales['x'].ticks namespace will allow users to aling ticks: 'start" for first (left) tick and 'end' for last (right) tick
Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

1 Suggestion that I think will make the functionality better and more flexible.

Also before this can be merged the typings for this property have to be adjusted here:

align: 'start' | 'center' | 'end';

And the documentation also needs to be updated here: https://github.com/chartjs/Chart.js/blob/master/docs/axes/cartesian/_common_ticks.md

src/core/core.scale.js Outdated Show resolved Hide resolved
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I like the outcome. It would be good to add some image based tests for this and to test how this works when labels are rotated. There are examples of those tests in https://github.com/chartjs/Chart.js/tree/master/test/fixtures/core.scale

src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
test/fixtures/core.scale/label-align-inner.js Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests. They look good. I think there's one more small case to consider that can be tested by hiding the Y axis (display: false). The default case for the left padding is half the width of the first label. When the alignment is set to 'inner' I think this should be 0

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Looks good to me. Needs a rebase though.

Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Think adding this else clause will make for a lot nicer looking option because the in between ticks get centered so you get this:
image

Which in my oppinion is a lot more pleasing to look at.

src/core/core.scale.js Outdated Show resolved Hide resolved
@Talla2XLC
Copy link
Contributor Author

Thanks for adding the tests. They look good. I think there's one more small case to consider that can be tested by hiding the Y axis (display: false). The default case for the left padding is half the width of the first label. When the alignment is set to 'inner' I think this should be 0

Add test for inner align without Y axis. Works perfect

@Talla2XLC
Copy link
Contributor Author

Talla2XLC commented Feb 7, 2022

Think adding this else clause will make for a lot nicer looking option because the in between ticks get centered so you get this: image

Which in my oppinion is a lot more pleasing to look at.

Just correct logic to handle central ticks for inner align and also correct test for this new look

@etimberg
Copy link
Member

etimberg commented Feb 7, 2022

Thanks for adding the tests. They look good. I think there's one more small case to consider that can be tested by hiding the Y axis (display: false). The default case for the left padding is half the width of the first label. When the alignment is set to 'inner' I think this should be 0

Add test for inner align without Y axis. Works perfect

They work, but there's a gap at the left and right edges in label-align-inner-onlyX.png. If you increase the length of the labels, that gap will increase shrinking the chart. By adjusting the paddingLeft and paddingRight calculations, this gap won't be present

@Talla2XLC Talla2XLC changed the title Add new align 'left-right' for X axis Add new align 'inner' for X axis Feb 7, 2022
@Talla2XLC
Copy link
Contributor Author

Thanks for adding the tests. They look good. I think there's one more small case to consider that can be tested by hiding the Y axis (display: false). The default case for the left padding is half the width of the first label. When the alignment is set to 'inner' I think this should be 0

Add test for inner align without Y axis. Works perfect

They work, but there's a gap at the left and right edges in label-align-inner-onlyX.png. If you increase the length of the labels, that gap will increase shrinking the chart. By adjusting the paddingLeft and paddingRight calculations, this gap won't be present

Got it! Padding for left and right borders was corrected. Tests results was corrected

kurkle
kurkle previously approved these changes Feb 9, 2022
etimberg
etimberg previously approved these changes Feb 9, 2022
types/index.esm.d.ts Outdated Show resolved Hide resolved
@Talla2XLC Talla2XLC dismissed stale reviews from etimberg and kurkle via d4bb967 February 10, 2022 16:51
Co-authored-by: Jacco van den Berg <39033624+LeeLenaleee@users.noreply.github.com>
@Talla2XLC
Copy link
Contributor Author

@etimberg , @kurkle , @LeeLenaleee
I'm not very familiar with open-source projects with restricted merge requests on Github so what to do next to merge this pull request? As I can see all participants approved this branch

@etimberg
Copy link
Member

So this is good to merge, but we want to release v3.7.1 first to keep the branching a little simpler. As soon as that is released (should be soon) then we'll merge this for 3.8.0

@etimberg
Copy link
Member

v3.7.1 has merged, so this is merging and will be released with v3.8.0

@etimberg etimberg merged commit 7c14ab7 into chartjs:master Feb 12, 2022
@etimberg etimberg linked an issue Feb 12, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New aligment for x axis ticks
4 participants