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

Canvas sizing incorrect without width/height and option for responsive #3

Open
mharmer opened this issue Jun 4, 2019 · 4 comments
Open

Comments

@mharmer
Copy link
Contributor

mharmer commented Jun 4, 2019

I'm trying to utilize the doughnut chart to convert some vue-chartjs over to Elm. We're trying to maintain a strict size of width="95px" and height="95px" and in VueJS we were using the responsive option set to false to enforce this.

ChartJS responsive doc: https://www.chartjs.org/docs/latest/general/responsive.html

I was expecting it to be included here: https://package.elm-lang.org/packages/blissfully/elm-chartjs-webcomponent/latest/Chartjs-Options

Some additional notes on the responsive mode: https://stackoverflow.com/a/43281488

Second, to get the sizing correct I also had to add a width and height attribute on the canvas element itself within webcomponent/chart.js (I believe this is to remove the defaults of 300 and 150 per https://stackoverflow.com/a/12019582), e.g.:

<div
  class="chart-container"
  style="position: relative; min-height: ${this.getAttribute("chartHeight")}px"
>
  <canvas
    width='${this.getAttribute("chartWidth")}px';
    height='${this.getAttribute("chartHeight")}px';
  ></canvas>
</div>

I'm not an expert on JS or canvas sizing, so it's possible I missed something obvious on how this might be already handled - I tried varying parent div attributes to adjust it to no avail. Adding the above didn't seem to break it in responsive mode.

I've attached my example: ElmDoughnutTest.zip that shows if I specify a 95 x 95 chart that it seems to always be responsive to the window resizing - which it could become very large.

@aaronwhite
Copy link
Contributor

@mharmer looks like I may have missed a configuration option for sure. Also, I'm no CSS/HTML/responsive expert myself, so it's quite possible there's some smoothing out to do here. I'm open to your suggestions, it hasn't yet come up in our (@Blissfully) own usage just yet.

@mharmer
Copy link
Contributor Author

mharmer commented Jun 4, 2019

@aaronwhite Thanks - did you want to tackle this, or should I look into creating a PR?

@aaronwhite
Copy link
Contributor

@mharmer - would just adding the responsive option unblock you here? I can look at what config options I may have missed and add, but would happily welcome any PRs as well

@mharmer
Copy link
Contributor Author

mharmer commented Jun 4, 2019

@aaronwhite I think both changes are needed, I tried to only add the responsive which helped a bit, but the default 300x150 seemed to be present still.

mharmer pushed a commit to mharmer/elm-chartjs-webcomponent that referenced this issue Jun 5, 2019
This allows users to provide an optional responsive boolean that
matches Chartjs' options.

Partially fixes issue vendrinc#3.
mharmer pushed a commit to mharmer/elm-chartjs-webcomponent that referenced this issue Jun 5, 2019
This is the final fix for vendrinc#3 to properly set the canvas size when
using a non-responsive chart. If responsive is used, this width
and height seem to be ignored and the chart should properly size
to the window dimensions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants