Skip to content

Add new align 'inner' for X axis - #10106

Merged
etimberg merged 8 commits into
chartjs:masterfrom
Talla2XLC:leftRightAlignForHorizontalAxis
Feb 12, 2022
Merged

etimberg merged 8 commits into
chartjs:masterfrom
Talla2XLC:leftRightAlignForHorizontalAxis

Conversation

@Talla2XLC

@Talla2XLC Talla2XLC commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

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

@LeeLenaleee LeeLenaleee left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread src/core/core.scale.js

@etimberg etimberg left a comment

Copy link
Copy Markdown
Member

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

Comment thread src/core/core.scale.js Outdated
Comment thread src/core/core.scale.js Outdated
Comment thread test/fixtures/core.scale/label-align-inner.js Outdated
Comment thread types/index.esm.d.ts Outdated
@Talla2XLC
Talla2XLC requested a review from LeeLenaleee January 31, 2022 18:28

@etimberg etimberg left a comment

Copy link
Copy Markdown
Member

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

@kurkle kurkle left a comment

Copy link
Copy Markdown
Member

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.

@LeeLenaleee LeeLenaleee left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/core/core.scale.js
@Talla2XLC

Copy link
Copy Markdown
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

Talla2XLC commented Feb 7, 2022

Copy link
Copy Markdown
Contributor Author

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

etimberg commented Feb 7, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Comment thread types/index.esm.d.ts Outdated
@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
Talla2XLC requested a review from etimberg February 10, 2022 18:19
@Talla2XLC

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

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