Improve view of variant history when a previous variant is selected #9782
Conversation
All images
|
|
Another possible improvement might be to highlight the currently-displayed variant in the list of variants when showing an old variant. Furthermore, when we are viewing an old variant, we could show the current one in the different color and show the open variant in the same color as the old ones. |
This is done in this PR... |
Haha, that will teach me to comment before actually running the code 😄 |
| @@ -24,13 +24,20 @@ | |||
| <% } %> | |||
| <% for (var i = 0; i < previous_variants.length; i++) { %> | |||
| <a | |||
| class="badge badge-secondary <%= collapseClass %>" | |||
| class="badge <%= previous_variants[i].variant_id == current_variant_id ? 'badge-light' : previous_variants[i].open ? 'badge-primary' : 'badge-secondary' %> <%= collapseClass %>" | |||
There was a problem hiding this comment.
I find the .badge-light is too easy to overlook, especially in comparison to the .badge-primary of the open variant. I actually think that there isn't a need to highlight the open variant, so I suggest changing the current variant to .badge-primary (or maybe .badge-info?) and removing the special handling of the open variant.
There was a problem hiding this comment.
Done in bec5ce0:
| @@ -74,7 +76,6 @@ WITH | |||
| LEFT JOIN variant_max_submission_scores AS vmss ON (vmss.variant_id = v.id) | |||
| WHERE | |||
| v.instance_question_id = $instance_question_id | |||
| AND NOT v.open | |||
| AND NOT v.broken | |||
There was a problem hiding this comment.
I almost wonder whether we should also include broken variants here? That would allow students to go back to any variant that they had seen, rather than hiding broken variants? I'm a bit torn being being transparent (showing broken variants) and keeping things simple (hiding broken variants).
There was a problem hiding this comment.
I think allowing a student to open a broken variant may cause more confusion than necessary. Besides, in some cases a broken variant will cause an issue to be created just by opening it.
There was a problem hiding this comment.
Yeah, we don't want to generate any more issues than we already do 😬
|
Note that #9851 includes another copy of the query. We'll need to at least remove |
Done in 8a90a70. |
|
@mwest1066 one question: as I was testing this, I noticed that single-variant questions still show the "All variants" prompt in the side panel. I believe this information is confusing and brings no additional information. Should I remove this if the question is single variant? |
Since there are cases where single variant may not be set but a single variant ends up being there in effect, and cases where the question changed from multi-variant to single-variant later, I decided to instead make the logic for this to show the list of variants in the score panel if there are other variants. This means that, in effect, if the current variant is the first and only variant, the "All variants" (or "Previous variants" in the master version) will not show, though creating a new variant will then start showing the list. I'm open to discussing other alternatives. The assessment overview is not affected, and always shows the list of variants, even if there is only one. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9782 +/- ##
==========================================
+ Coverage 66.43% 66.44% +0.01%
==========================================
Files 453 453
Lines 70289 70319 +30
Branches 5647 5653 +6
==========================================
+ Hits 46694 46723 +29
Misses 23171 23171
- Partials 424 425 +1 ☔ View full report in Codecov by Sentry. |
| <%# Only show previous variants if the question allows multiple variants, and there is at least one variant other than the current one %> | ||
| <% if (instance_question_info.previous_variants?.some((previous_variant) => previous_variant.id != locals.variant?.id)) { %> |
There was a problem hiding this comment.
I think the comment is now slightly wrong (we don't require the question to allow multiple variants).
I think that we should also show "All variants" if the question allows multiple variants, even if we don't currently have any old variants. It's already clear from the UI that other variants are possible in principle, so I think it's helpful to explicitly show that there aren't any others. What do you think? |
I'm ok with that. For completeness I included the case where single-variant was false at some point in the past, such that multiple variants exist. See latest commit. |
Follow-up to #6105, resolves #9780. Minor improvements to the view of the variant history, especially when viewing a previous history. Namely:
One thing to consider: when a new variant is created, it will not be shown immediately in the variant list, since the variant is created after the query that finds previous variants. It will be shown as soon as the page is refreshed or a submission is made, or when going to the assessment overview. Should this be changed to either (a) always show this variant or (b) show only open variants with at least one submission?