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

Upgrade Chart.js to 3.7.0 #17

Merged
merged 5 commits into from
Jan 7, 2022
Merged

Conversation

ggrossetie
Copy link
Member

@ggrossetie ggrossetie commented Jan 5, 2022

What I Did

  • Upgrade Chart.js from 1.0.2 to 3.7.0 and update code accordingly
  • Use https protocol (instead of http) when loading scripts from cdnjs.cloudflare.com
  • Apply RuboCop to asciidoctor-chart.gemspec
  • Add rubygems_mfa_required in gemspec
  • Preserve units on the width and height attribute (if unit is absent, assume the size is in pixels "px")
  • Disable maintainAspectRatio when the width or height is defined. From the documentation: "Note that in order for the above code to correctly resize the chart height, the maintainAspectRatio option must also be set to false."
  • Set position: relative; on the container
  • Add a class on the container chartjs-container. From the documentation: "Detecting when the canvas size changes can not be done directly from the canvas element. Chart.js uses its parent container to update the canvas render and display sizes. However, this method requires the container to be relatively positioned and dedicated to the chart canvas only."

resolves #8 and partially resolves #9

//cc @jpminnovation

@ggrossetie ggrossetie marked this pull request as ready for review January 5, 2022 14:47
@ggrossetie ggrossetie requested a review from mojavelinux January 5, 2022 14:47
# 'changelog_uri' => 'https://github.com/asciidoctor/asciidoctor-chart/blob/master/CHANGELOG.adoc',
'community_chat_uri' => 'https://asciidoctor.zulipchat.com',
'source_code_uri' => 'https://github.com/asciidoctor/asciidoctor-chart',
'rubygems_mfa_required' => 'true'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to rubygems/rubygems.org#2500 (comment) you should enable MFA on https://rubygems.org/ and use "UI and gem signin".

In theory, it would be possible to publish using an API key (from CI/CD) even though MFA is enabled (and required).

@ggrossetie
Copy link
Member Author

ggrossetie commented Jan 5, 2022

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

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

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

the parent container has a strict width of 500px

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

@ggrossetie
Copy link
Member Author

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

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

@ggrossetie ggrossetie merged commit 090e9f1 into asciidoctor:master Jan 7, 2022
@ggrossetie ggrossetie deleted the chartjs-3-7-0 branch January 7, 2022 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants