-
Notifications
You must be signed in to change notification settings - Fork 26
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
adds data tables to more course visualizations #7946
base: master
Are you sure you want to change the base?
adds data tables to more course visualizations #7946
Conversation
328508d
to
dfceb22
Compare
this course was used for the screen shot above ... http://localhost:4200/data/courses/2224 2224 is the course ID |
@dartajax - not sure where we left it on this particular aspect. render a donut here instead, or not? |
also - the graph is "staying within it's quadrant", but it's bumping up against the next one. i could give it a bit of room though @dartajax - given some extra room - would that cut it for you? |
I think I like the bars myself but have put it up for team discussion - it seems like more information is available sooner - and I like that. |
Why not just make it straight quadrants (quarters) of the screen available to each visualization / graph in case we end up converting the Instructors to be bar chart as well? The extra room is helpful though and would cut it for me. |
i like this, will give it a try. thanks for the suggestion. |
4b4ff66
to
b159f00
Compare
6f280e2
to
fc46a82
Compare
c9ce955
to
5f44c7f
Compare
bar chart across the board.
we have no way to present them in the graph, so let's not include them at all.
convert course/instructor/session-types graph from donut to the more appropriate bar-chart.
- destructuring code to make it a bit more readable - chain methods where possible to reduce cognitive load introduced by unnecessary local vars - ensure that tooltip content is always html safe
…ith data-table tests, filter out zero-data elements.
this required further consolidation of data-loading getters to make the loading state check reliable - otherwise the "no data" message will blink in while data is still being loaded. i've added a spinner while at it since there's a noticable delay with content rendering.
we need to distringuish between unlinked target entities, and linked target entities without instructional time. only show a "no data" message if there's nothing linked at all. while at it - fix content of flash while data is loading by consolidating getters. adds links to sub-visualizations in the data tables.
5825fd9
to
5946d53
Compare
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.
Nice test detail, I didn't validate the data manipulation because my eyes glazed over after the first few, but given the extensive click testing so far and the test coverage I think this is ready. I made a comment about slice
that doesn't need to be part of this change, more for informational purposes.
async getData(sessions) { | ||
if (!sessions) { | ||
async getData(course, user) { | ||
const sessions = (await course.sessions).slice(); |
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.
Calling .slice()
on Ember data loaded relationships isn't necessary anymore. They act just like an array and can be passed to RSVP.*
methods without needing to be sliced first.
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.
good catch. general cleanup-ticket and PR incoming after this lands.
corrected ✔️
looking good! I think we have arrived for this PR. remaining issues will be follow-ons and up for discussion. |
fixes ilios/ilios#5584
fixes ilios/ilios#5583