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

Fix chart having hidden right padding #5190

Closed
wants to merge 1 commit into from

Conversation

jhaenchen
Copy link
Contributor

@jhaenchen jhaenchen commented Jan 26, 2018

This PR fixes an issue wherein a chart displayed without ticks allows a point to go halfway out of the chart canvas area. Instead of adding padding when ticks are enabled (see removed lines) we add a small offset to the available axis area so 5px is reserved at the end of the axis.

Two things:

  • I'm not sure if this change needs to be made per scale type.
  • I'm not sure if using a static offset is sufficient. Given that point width is customizable, it would be better to offset the axis by the max point width + 1.

Before:
https://codepen.io/anon/pen/baXVWX
image

After:
image

@jhaenchen jhaenchen changed the title Add offset to axis length instead of padding to chart area Fix chart having hidden right padding Jan 26, 2018
@jhaenchen
Copy link
Contributor Author

This also offsets the last label, making it not centered I think. Will look into this.

@etimberg
Copy link
Member

I'm totally OK with removing the 3px padding in the normal case (can't really recall why it's there).

I'm not sure that a hard-coded 5 is the correct way to do this though for the reasons you outlined. (and it would need to be applied to all axes, in both directions). One potential solution is to perhaps allow the overall padding option to be a function that returns the padding on each edge. Then users could attach a function that would deduce the correct padding based on the points and their sizes. It would also mean that the axes would not need to be changed which has some benefits.

@simonbrunel @benmccann thoughts on this idea?

@jhaenchen
Copy link
Contributor Author

The issue here is that I have a chart that should extend to the full width of the container, as its padding is set to zero. However, with ticks enabled but not rotated, a default padding of 7.35px was added to the chart edge. When I disable this padding on its own, my last point goes half off the chart instead of being fully displayed. I want the axis to extend the full width of the container and the last point to be fully displayed, so I added a 5px dead zone to the axis at the end so the line continued but points do not. However, this offset could be overcome by a sufficiently large point radius.

Regardless, I think this is well within the realm of the chart's fit logic and should probably be dealt with there.

@jhaenchen
Copy link
Contributor Author

Would love some suggestions on how to fix this. I think the last point should be placed at right side - its radius - 2px extra offset. If it goes all the way to the end it'll go off the chart edge.

@benmccann
Copy link
Contributor

@jhaenchen would you be able to rebase this PR? thanks!

@jhaenchen
Copy link
Contributor Author

@benmccann I can rebase but this still needs a solution and I really have no idea how to approach

@benmccann
Copy link
Contributor

@jhaenchen I think the solution will need to be in the line controller since it's the only thing that's aware of the point's width. It looks to me like maybe we handle it already on the top and bottom of the chart and just need to handle it on the right and left as well. Look at how we adjust by halfBorderWidth:

https://github.com/chartjs/Chart.js/blob/master/src/controllers/controller.line.js#L309

@jhaenchen jhaenchen reopened this Aug 21, 2018
@jhaenchen
Copy link
Contributor Author

@benmccann I have applied your suggested fix. One concern I had is in regard to the existing code which seems to subtract half the border width from the top and bottom properties of the chart area. In the Canvas coordinate system, wouldn't that expand the chart area instead of shrink it? I'm not sure so I used the arithmetic used for top and bottom, but I wanted to see if you could just confirm my thinking.

@benmccann
Copy link
Contributor

@jhaenchen in the canvas, top left is (0,0). So saying top: area.top - halfBorderWidth, is giving a smaller number, which is moving closer to the top (i.e. expanding the area)

@benmccann
Copy link
Contributor

@jhaenchen this looks good to me. Can you share an updated before/after screenshot?

@benmccann
Copy link
Contributor

@jhaenchen would you be able to rebase this PR? If you can share a before and after screenshot that'll help get this merged

@jhaenchen
Copy link
Contributor Author

Okay guys I’ll rebase this one more time. It’s been 11 months and I know I’m partially responsible for that delay but this is getting a bit silly.

@benmccann
Copy link
Contributor

@jhaenchen just a reminder that the ball is still in your court. I know this PR's been open for a long time. I'd really love to close it out, but unfortunately we can't merge it as long as there are merge conflicts

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.

3 participants