-
Notifications
You must be signed in to change notification settings - Fork 137
Migrate/achievement custom bar chart #412
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
base: 9.x
Are you sure you want to change the base?
Migrate/achievement custom bar chart #412
Conversation
Pasindufdo98
left a comment
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.
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. |
Pasindufdo98
left a comment
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.
Approved. Good work!
disururathnayake
left a comment
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.
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: