Migrate/achievement custom bar chart#412
Migrate/achievement custom bar chart#412lachlan-robinson wants to merge 11 commits intothoth-tech:9.xfrom
Conversation
Pasindufdo98
left a comment
There was a problem hiding this comment.
Hi @Lachalan nice work on this feature. I was able to successfully build without any build errors. The chart looks clean, and the tooltips are a great touch. The only thing I noticed is that there’s a console error (Cannot read properties of undefined (reading 'selectAll')). It might be worth adding a quick null/undefined check before calling selectAll in renderChart() just to avoid that runtime issue. Other than that, everything looks solid!
|
Hey @lachlan-robinson, Good work on the migration. No obvious console or API errors. The template has been ported to Angular Material. The graph itself looks good. Tooltips are a welcome feature. The only suggestion I can make is to do with performance. Currently, it seems that the tooltip updates on mouse move. This slows down when moving the mouse a lot. I'd suggest anchoring the tooltip to the part of the graph that is being hovered over and rendering it once Approving this, great work. |
disururathnayake
left a comment
There was a problem hiding this comment.
Hey @lachlan-robinson ,
Great job on the migration! I didn’t spot any console or API errors and the graph looks good. The addition of tooltips is a nice touch too. Keep it up!
Description
Migration of the achievement-custom-bar-chart component in the 9.x branch.
Includes update to the package.json for various d3 modules necessary for creating the custom graph.
IMPORTANT: This migration is branched from the pending migration of the outcome service: #390
Type of change
How Has This Been Tested?
Before:
After:
Testing Checklist:
Checklist: