asciidoctor / asciidoctor-chart

A set of Asciidoctor extensions that add a chart block and block macro to AsciiDoc for including charts in your AsciiDoc document.
Other
30 stars 13 forks source link

Upgrade Chart.js to 3.7.0 #17

Closed ggrossetie closed 2 years ago

ggrossetie commented 2 years ago

What I Did

resolves #8 and partially resolves #9

//cc @jpminnovation

ggrossetie commented 2 years ago

Example

== Chart.js

=== Default
chart::examples/sample-data.csv[line,engine=chartjs]

=== width=500
chart::examples/sample-data.csv[line,engine=chartjs,width=500]

=== height=200
chart::examples/sample-data.csv[line,engine=chartjs,height=200]

=== width=500,height=100
chart::examples/sample-data.csv[line,engine=chartjs,width=500,height=100]

=== width=40%,height=100
chart::examples/sample-data.csv[line,engine=chartjs,width=40%,height=100]

=== width=80vw,height=100
chart::examples/sample-data.csv[line,engine=chartjs,width=50vw,height=100]

=== width=100%,height=200px
chart::examples/sample-data.csv[line,engine=chartjs,width=100%,height=200px]

Result

As you can see below, charts with a width of 500px won't resize when the with is smaller than 500px. Using max-width: 500px instead of width: 500px would solve this issue. Should we set max-width and max-height instead of width and height?

Desktop (1024px)

Mobile (425px)

mojavelinux commented 2 years ago

As you can see below, charts with a width of 500px won't resize when the with is smaller than 500px.

Do you know the reason for this? Is it conflicting with CSS added by the chart library?

ggrossetie commented 2 years ago

I don't think so, the library is using a <canvas> and the parent container has a strict width of 500px so it won't resize if the screen size is smaller (i.e., the element overflows so you do get an horizontal scrollbar on the page). I think that's the expected behavior with fixed width.

mojavelinux commented 2 years ago

the parent container has a strict width of 500px

The parent container added by the library or the page template?

ggrossetie commented 2 years ago

The parent container is added by the page template in this extension. As far as I know, Chart.js does not create any HTML element, it uses the existing <canvas> element (also added by the page template).

So we do have full control over the HTML. In other words, nothing prevents us from using max-width. In my opinion, it would be better because it will provide a better out-of-the-box experience (it's probably better to have a responsive chart by default rather than an overflowing chart).

But if we do that, do we want to introduce a "fixed-width" attribute in case you really want a fixed width? I don't think we should do (unless someone makes a strong argument for it).

Regarding height I don't think there's any issue using height but we could use max-height as well for consistency.

Does it sound reasonable?

mojavelinux commented 2 years ago

Yep, sounds reasonable to me! Thanks for the clarification.