-
Notifications
You must be signed in to change notification settings - Fork 0
Add charts.js package and example components #23
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to merge to me, nice! Remaining problems could be tackled in smaller followup steps
}, | ||
}; | ||
new Chart( | ||
(document.getElementById("myCanvas") as HTMLCanvasElement).getContext( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct / safe as it is, because onMount
runs after the DOM elements below the script section are ready. But optionally, you could use https://learn.svelte.dev/tutorial/actions and/or a let canvas: HTMLCanvasElement;
and <canvas bind:this={canvas} />
instead of getElementById
. The danger of id
is that the IDs need to be unique for the whole page, so no other component can use this ID, or you can't have two ApiBartChart
components on one page.
In fact, looks like BarChart.svelte
is using actions as I'd expect. Could we do the same thing here to nudge people towards best practices? The complication is that the API results won't be immediately ready... Hmm, the simplest approach I know is to keep onMount
as you have it, but get rid of id
(for the uniqueness problem) and use bind:this
instead
And it could be worth it to have a comment about why doing this instead of the use:
actions is helpful, to guide people to doing the appropriate thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation and thoughts on this. I've updated as you suggested with a comment and the bind:this
approach.
Thanks @dabreegster for reviewing this, opened #28 for further work on this and will merge now. |
Closes #6.
This PR adds:
chart.js
dep (svelte-chartjs
does not seem to be supported with svelte5 at the moment)ua_components
package and allow data to be passed in, otherwise use svelte-chartjschartjs-zoom-plugin
)