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

Chartjs taking canvas width and height attribute, but ignoring css width height. #3378

Closed
gunins opened this issue Sep 28, 2016 · 9 comments
Closed

Comments

@gunins
Copy link

gunins commented Sep 28, 2016

Chartjs taking canvas width and height attribute, but ignoring css width height. Also there should be an option, where I can add width and height programmatically, true options.

@etimberg
Copy link
Member

@simonbrunel thoughts here? I think this is a duplicate of another issue

@simonbrunel
Copy link
Member

For non responsive charts, some work has been done in #3356: style width and height are now used to initialize the canvas size if width and height attributes are not specified, so this should work:

<canvas style="width: 512px; height: 256px"></canvas>

BUT relative size does NOT work (e.g. style="width: 50%; height: 25vh"), neither programmatically change the canvas CSS size, because we can't detect that dynamic changes. Can you post pseudo code of what you want to do exactly?

@gunins
Copy link
Author

gunins commented Sep 30, 2016

For standard should work either, css canvas {width:500px; height:300px;}

Also should be very useful, if responsive:false; and not suport dynamic sizes such as width:100%;, let user to define size options like {width:500px,height:300px} also prototype method .resize() should take optional params width and height.

@simonbrunel
Copy link
Member

@gunins canvas {width:500px; height:300px;} should work with #3356 (merged), but only when responsible: false. You can have a look at the unit tests to figure out most of the supported implementations. Would be great to have some beta testing on these changes ;)

I totally agree with your second remark and I already started to rework the resize() method:

  • if responsive: false, calling chart.resize() (without arguments) should resync the canvas render size (canvas.width,height) based on the current display size (canvas.style.width,height),
  • if responsive: false, calling chart.resize(width, height) should change the display size and resync the render size,
  • if responsize: true, keep the current implementation

What do you think?

@gunins
Copy link
Author

gunins commented Sep 30, 2016

Sure when responsive:true width and height should be ignored. I also would like to see availability to manage in config width and height. like

             new Chart(ctx,{{
                        responsive: false,
                        width:500,
                        height:300,
                        scaleShowGridLines: false,
                        showScale: false,
                        maintainAspectRatio: this.maintainAspectRatio,
                        barShowStroke: false,
                    }});

P.S. This is kind of regards to different issue, but related to this one.
Maybe even, because for responsive use iframe, and it's quite heavy, responsive implement as module, instead of in core. Because for newer bowsers has Mutation Observer support https://developer.mozilla.org/en/docs/Web/API/MutationObserver#MutationObserver(); this will give custom opportunity to implement responsive behaviour.

@simonbrunel
Copy link
Member

What is your exact use case because I would not recommend to add these extra height and width options. It will make the size decision logic even more complex and confusing for something already managed outside the lib:

// From HTML
<canvas width="300" height="200"></canvas>
<canvas style="width: 300px; height: 200px"></canvas>

// Set size on chart creation
canvas.style.width = '300px';  // or canvas.width = 300;
canvas.style.height = '200px'; // or canvas.heigth = 200;
new Chart(canvas, { options: { responsive: false }});

// Change the display size
chart.resize(300, 200);

// Resync the render size
chart.resize();

/cc @chartjs/maintainers

@gunins
Copy link
Author

gunins commented Sep 30, 2016

Reason for custom width and height is quite simple. Because Chartjs not support %, I can programmatically get with and height by pixels. But instead of DOM method element.setAttribute will be easier add dimensions inside options, than extra step. If in options width and height is defined. This should override style and attribute, of course for responsive:true, no need for width, height params.

@gunins
Copy link
Author

gunins commented Sep 30, 2016

Actually is there is any reason to take attribute width instead of clientHeight, clientWidth
those attributes are generated based on actual canvas size, even in css is %

@simonbrunel
Copy link
Member

clientHeight and clientWidth include padding, so definitely not the values we want. But if you look at the current code, you will see that we don't simply read css values, we read the computed/used size. This size is always in pixels (except if your canvas is not displayed), so that means the current code works as long as your parent container size does not change.

<canvas style="width: 50%; height: 50vh"></canvas>

I should have been more precise when I said "it doesn't work with relative sizes", because you can use css sizes in % (or vh, vw) with responsive: false, if you manage to manually call chart.resize() (no parameters) to resync the canvas render size when the parent container size change (first I need to rework the resize method as explained above).

If your parent container size doesn't change, then it should already work as you expect :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants