chartist-js / chartist

Simple responsive charts
https://chartist.dev
MIT License
13.34k stars 2.53k forks source link

feat: add an error message when chart container is not found #1392

Closed Arantiryo closed 1 year ago

Arantiryo commented 1 year ago

Fixes #1354.

Error message example: no-container-error-chartist

codecov-commenter commented 1 year ago

Codecov Report

Merging #1392 (1f33c00) into main (89bcbb5) will decrease coverage by 0.17%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1392      +/-   ##
==========================================
- Coverage   74.41%   74.24%   -0.18%     
==========================================
  Files          42       42              
  Lines        1247     1250       +3     
  Branches      326      328       +2     
==========================================
  Hits          928      928              
- Misses        238      241       +3     
  Partials       81       81              
Impacted Files Coverage Δ
src/charts/BaseChart.ts 44.82% <0.00%> (-0.79%) :arrow_down:
src/core/creation.ts 91.54% <0.00%> (-2.66%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

gionkunz commented 1 year ago

Awesome thats long overdue 😂👍

gionkunz commented 1 year ago

In strict typescript container always needs to be defined. I would anyway catch the error earlier before this function call because then you can also print hinting information like the selector that was used.

Arantiryo commented 1 year ago

Good suggestion @gionkunz, thanks for pointing that out!

Since we can't catch get the selector right before this function call I figured we could modify the error message in the BaseChart.ts so the both error messages would look like this:

no-container-error-msg

What do you think?

gionkunz commented 1 year ago

Cool! We had many issues open because people did not understand this error.

What about the size limit check? Do we need to adjust the threshold?

Arantiryo commented 1 year ago

Probably. Let me kindly ask @dangreen about this, he knows way more about CI / CD than I do. :)