create new universal js config file to include into the skill wheel t…#659
create new universal js config file to include into the skill wheel t…#659
Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes the Radial Bar Chart (“Skills Wheel”) configuration and render helper into a shared static JS file, then updates templates to use that shared function rather than repeating the config inline.
Changes:
- Added
main/static/main/js/radial-bar-chart-config.jsto holdradialBarChartConfigandrenderRadialBarChart(...). - Updated three templates to load the shared JS and call
renderRadialBarChart(...)instead of duplicating the config object. - Reduced inline JS duplication across the Skills Wheel pages/snippets.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| main/templates/main/user_skill_profile.html | Loads shared chart config JS and switches to renderRadialBarChart(...). |
| main/templates/main/snippets/hero.html | Loads shared chart config JS and reuses renderRadialBarChart(...) for the homepage hero wheel. |
| main/templates/main/pages/roles.html | Loads shared chart config JS and reuses renderRadialBarChart(...) to render all sample role wheels. |
| main/static/main/js/radial-bar-chart-config.js | New shared chart configuration + render helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
AdrianDAlessandro
left a comment
AdrianDAlessandro
left a comment
There was a problem hiding this comment.
Great! I much prefer this approach of using a template snippet.
I think we can unify the modes with a bit of logic and not need to provide a mode at all. Certainly the skill-profile and hero mode should be able to use the exact same logic. They both take data for one user and produce one plot. And then following that, the data for one user can just be in a list of length one, and that's the only different in the roles mode (it takes a list of user data instead of just one).
Does that make sense?
| context["sample_data"] = sample_data | ||
| context["chart_data"] = dumps( | ||
| [ | ||
| {"target_id": str(p["user_id"]), "user_data": p["user_data"]} | ||
| for p in sample_data | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Not a huge fan of this structure. We're doubling up on the context data being sent. Since the "user_data" values are in both context entries. It also means that in the templates, the id value of the div is coming from the "sample_data", but the id that the JS is updating is coming from the "chart_data", which feels unstable to me.
Perhaps, instead of adding an entirely new context variable to all of the views, the best option is to use the existing "user_id" field of the sample data. This just means adding that value in to the skill profile view.
There was a problem hiding this comment.
This is still sending a separate sample_data that also contains the user_data. We should only need to send one of them.
There was a problem hiding this comment.
I missed this I still had sample_data in the HTML file just changed it now.
AdrianDAlessandro
left a comment
AdrianDAlessandro
left a comment
There was a problem hiding this comment.
Great! Happy with this now!
Description
I removed the common bits of JS code and moved them to a JS file in the static folder and Included it in the skill profile page, roles and index page (hero.html)
Fixes #595
Type of change
Key checklist
python -m pytest)mkdocs serve)pre-commit run --all-files)Further checks