Improve UX of evaluation show page #5636
Conversation
jorg-vr
added
the
enhancement
|
What do you think about vertically aligning the text? |
There was a problem hiding this comment.
I'm not sure about this. The current design didn't come out of nowhere and was the result of many iterations taking into account evaluations with multiple columns (in the overview), and limited v-space (on an evaluation itself). I'll first revisit the original iterations (at a later time) before reviewing this.
|
Bart, I don't remember what were the discussed requested changes for this pr? I do remember discussing that the changes where now much less important due to the 'evaluate' buttons at the top. |
|
I recall
|
WalkthroughThe recent changes primarily focus on enhancing the rendering logic and layout of feedback statuses in the evaluations interface. Modifications include the integration of SVG icons, the removal of unnecessary conditional logic, and the restructuring of user feedback rows into a list format. Additionally, a paragraph element displaying evaluation details has been removed, streamlining the presentation. These updates aim to improve the overall clarity and usability of the evaluation interface. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
What do you think of this?
Keeps the checkmark with comment icon used in other places. But also uses the known submission state icons, thus separating the two types of information into two columns. |
|
Vission for changes (dutch from Bart):
Todo:
|
|
I am not fully convinced of the all blue approach. But I don't have a very strong opinion on either of the three options. For reference: All grey: Mixed: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/views/feedbacks/_user_feedback_row.html.erb (1)
6-13: Consider adding ARIA attributes for better accessibilityThe implementation looks good and aligns with the PR objectives to improve the feedback status display. The ellipsis overflow with title attribute is a good approach for handling long names.
Consider adding ARIA attributes to improve accessibility:
-<td class="ellipsis-overflow name" title="<%= feedback.exercise.name %>"> +<td class="ellipsis-overflow name" title="<%= feedback.exercise.name %>" aria-label="Exercise: <%= feedback.exercise.name %>">app/views/evaluations/_feedback_status.html.erb (2)
1-4: Enhance accessibility for the feedback status linkConsider adding ARIA attributes to improve accessibility for screen readers.
<%= link_to evaluation_feedback_path(evaluation, feedback), title: feedback_title(feedback), "data-bs-toggle": "tooltip", + "aria-label": feedback_title(feedback), + "aria-describedby": "feedback-status-tooltip", class: "btn btn-icon #{local_assigns.fetch(:active, false) ? 'disabled' : ''}" do %>
14-14: Consider using relative positioning for the background circleThe hardcoded position (cx="21" cy="22.25") might not scale well with different viewport sizes.
Consider using percentages or viewBox units for positioning:
- <circle cx="21" cy="22.25" r= "9" fill="var(--d-surface)"></circle> + <circle cx="87.5%" cy="92.7%" r="37.5%" fill="var(--d-surface)"></circle>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/views/evaluations/_feedback_status.html.erb(1 hunks)app/views/evaluations/show.html.erb(0 hunks)app/views/feedbacks/_user_feedback_row.html.erb(1 hunks)
💤 Files with no reviewable changes (1)
- app/views/evaluations/show.html.erb
🔇 Additional comments (3)
app/views/feedbacks/_user_feedback_row.html.erb (2)
14-15: LGTM!
The table structure is properly closed.
11-11: Verify that the new feedback status implementation addresses user confusion
Based on the PR objectives, users previously struggled to understand the meaning of status icons. Let's verify that the new implementation in evaluations/feedback_status partial addresses this concern.
✅ Verification successful
The verification results show that the new feedback status implementation has made several improvements to address user confusion:
New implementation effectively addresses user confusion with status icons
The new feedback status partial (_feedback_status.html.erb) implements a clear visual system:
- Uses descriptive icons (
mdi-comment-check-outlinefor completed feedback,mdi-comment-outlinefor pending) - Includes submission status through a composite icon design
- Provides tooltips for additional clarity (
data-bs-toggle: "tooltip") - Maintains consistency with the rest of the application's tooltip usage pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the feedback_status partial for improved icon implementation
# and verify removal of help text mentioned in PR objectives
# Check the feedback_status partial implementation
cat app/views/evaluations/_feedback_status.html.erb
# Look for any remaining help text that should have been removed
rg -g "app/views/**/*.erb" "help.*text|tooltip|hint"
Length of output: 8951
app/views/evaluations/_feedback_status.html.erb (1)
1-24: Verify that the new implementation addresses user confusion
While the implementation technically combines comment and submission status icons, consider these suggestions to further improve user experience based on the PR objectives:
- The
btn-iconclass might not sufficiently indicate clickability - The tooltip might be missed by users who don't hover
Consider these improvements:
- Add a subtle hover effect or button-like styling
- Include a visual affordance (like a subtle outline) to indicate clickability
- Consider adding a small "Click to evaluate" label for new users
I prefer the mixed colors, because of the visual distinction. |
There was a problem hiding this comment.
Copilot reviewed 5 out of 10 changed files in this pull request and generated no suggestions.
Files not reviewed (5)
- app/assets/stylesheets/components/evaluations.css.scss: Language not supported
- app/assets/stylesheets/components/table.css.scss: Language not supported
- app/assets/stylesheets/models/activities.css.scss: Language not supported
- app/views/evaluations/_feedback_status.html.erb: Evaluated as low risk
- app/views/evaluations/_grade_status.html.erb: Evaluated as low risk
Comments skipped due to low confidence (1)
app/helpers/evaluation_helper.rb:17
- [nitpick] Consider using
feedback.submission.nil?instead offeedback.submission.blank?for clarity.
submission_status_text = if feedback.submission.blank?
|
@bmesuere de besproken opties: Enkel checkmark ingekleurd: Volledig icoontje ingekleurd: Momenteel heb ik de eerste optie gekozen. |
|
ik heb de feeback tabel nogmaals aangepast. Bold of ander font kleur werkte niet goed. Na nog even zoeken leek de functionaliteit wel hard op die van de
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
app/assets/stylesheets/components/evaluations.css.scss (2)
156-199: Consider providing a dense variant for the feedback list.The implementation nicely improves accessibility and clickability. However, the fixed height of 56px per item might create excessive whitespace in lists with many items.
Consider adding a dense variant:
ul.feedback-list { // ... existing styles ... + &.dense li a { + height: 40px; + border-radius: 20px; + } }
210-222: Consider refactoring to avoid !important declarations.The use of
!importantflags suggests potential CSS specificity issues that could make the styles harder to maintain or override when needed.Consider increasing specificity naturally:
-.feedback-icon { - color: var(--d-on-surface-muted) !important; +.feedback-metadata .feedback-icon { + color: var(--d-on-surface-muted); - i.completed { - color: var(--d-secondary) !important; + i.completed { + color: var(--d-secondary); } }app/views/evaluations/_feedback_status.html.erb (2)
3-5: Add aria-hidden to decorative iconsThe Material Design icons should be hidden from screen readers as they are decorative.
<foreignObject x="0" y="0" height="24" width="24"> - <i class="mdi mdi-comment-outline"></i> + <i class="mdi mdi-comment-outline" aria-hidden="true"></i>
7-9: Improve completion status icon accessibilityThe checkmark icon should be hidden from screen readers and its meaning should be conveyed through the parent SVG's aria-label.
<foreignObject x="6" y="1" height="16" width="12"> - <i class="mdi mdi-check-bold mdi-12 completed"></i> + <i class="mdi mdi-check-bold mdi-12 completed" aria-hidden="true"></i>app/views/evaluations/_evaluation_table.html.erb (1)
18-23: Excellent UX improvements addressing user feedback!The changes effectively address the UX issues identified in user testing:
- Icons are now clearly clickable with button styling
- Tooltips provide clear feedback about functionality
- Proper semantic markup with button classes improves accessibility
Consider these additional enhancements:
<%= link_to evaluation_feedback_path(evaluation, feedback), title: feedback_title(feedback), "data-bs-toggle": "tooltip", - class: "btn btn-icon d-btn-secondary" do %> + class: "btn btn-icon d-btn-secondary", + aria-label: t('.evaluate_submission', exercise: feedback.exercise.name, student: feedback.user.full_name) do %>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/assets/stylesheets/components/btn.css.scss(1 hunks)app/assets/stylesheets/components/evaluations.css.scss(1 hunks)app/views/evaluations/_evaluation_table.html.erb(1 hunks)app/views/evaluations/_feedback_status.html.erb(1 hunks)app/views/feedbacks/_user_feedback_row.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/feedbacks/_user_feedback_row.html.erb
🔇 Additional comments (4)
app/assets/stylesheets/components/evaluations.css.scss (1)
201-206: LGTM! Clean implementation of icon button hover states.
The hover effect provides clear visual feedback while maintaining consistency with Material Design principles.
app/assets/stylesheets/components/btn.css.scss (1)
387-391: LGTM! Well-structured secondary button implementation.
The implementation follows the established pattern and properly integrates with the theming system. This addition supports the PR's goal of improving button clarity.
app/views/evaluations/_feedback_status.html.erb (2)
11-18: 🛠️ Refactor suggestion
Enhance submission status implementation
The current implementation needs improvements for better error handling and accessibility.
- <circle cx="21" cy="22.25" r= "9" class="circle"></circle>
+ <circle cx="21" cy="22.25" r="9" class="circle" aria-hidden="true"></circle>
<foreignObject x="12" y="12" height="18" width="18">
<% if feedback&.submission.present? %>
- <%= submission_status_icon feedback.submission %>
+ <%= submission_status_icon(feedback.submission) rescue content_tag(:i, '', class: 'mdi mdi-18 mdi-alert text-warning', title: t('.status_error'), 'aria-hidden': true) %>
<% else %>
- <i class="mdi mdi-18 mdi-cancel"></i>
+ <i class="mdi mdi-18 mdi-cancel text-muted" aria-hidden="true"></i>
<% end %>Likely invalid or redundant comment.
1-1: 🛠️ Refactor suggestion
Enhance SVG accessibility attributes
The SVG element should include proper ARIA attributes for accessibility.
-<svg viewBox="0 0 24 24" width="24px" height="24px" style="overflow: visible" class="feedback-icon" >
+<svg viewBox="0 0 24 24" width="24px" height="24px" class="feedback-icon overflow-visible" role="img" aria-label="Feedback Status">Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/assets/stylesheets/components/evaluations.css.scss (1)
201-206: Consider adding transition for smoother hover effectThe hover effect on button icons could benefit from a smooth transition.
.btn-icon { + .feedback-icon .circle { + transition: opacity 0.2s ease; + } &:hover, &:focus { .feedback-icon .circle { opacity: 0; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/assets/stylesheets/components/btn.css.scss(1 hunks)app/assets/stylesheets/components/evaluations.css.scss(1 hunks)app/views/evaluations/_evaluation_table.html.erb(1 hunks)app/views/evaluations/_feedback_status.html.erb(1 hunks)app/views/feedbacks/_user_feedback_row.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/feedbacks/_user_feedback_row.html.erb
- app/views/evaluations/_feedback_status.html.erb
🔇 Additional comments (4)
app/views/evaluations/_evaluation_table.html.erb (1)
18-23: Good improvement to button accessibility and clarity!
The changes effectively address the UX issue where users didn't recognize icons as clickable buttons. The addition of tooltips and consistent button styling enhances discoverability.
Let's verify the tooltip text implementation:
✅ Verification successful
Good improvement to button accessibility and clarity!
The tooltip implementation is correctly handled through the feedback_title helper which provides appropriate text based on the submission status:
- For submissions without content, it shows a "no submission" message
- For submissions with content, it shows the human-readable submission status (correct, wrong, running, etc.)
The implementation aligns well with the UI improvements by providing clear, contextual information through tooltips while maintaining consistent button styling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the feedback_title helper implementation
rg -A 5 "def feedback_title"
Length of output: 557
Script:
#!/bin/bash
# Check the translation keys for feedback status
rg "evaluations\.feedback_status\.no_submission" config/locales/
Length of output: 66
Script:
#!/bin/bash
# Let's check the enum definitions in the Submission model and their translations
rg -A 5 "enum.*status" app/models/
rg "submission\.status\." config/locales/
Length of output: 2477
Script:
#!/bin/bash
# Let's check the translations for submission statuses and feedback
rg -g "*.yml" "submission:" config/locales/ -A 10
rg -g "*.yml" "evaluations:" config/locales/ -A 10
Length of output: 39166
app/assets/stylesheets/components/evaluations.css.scss (2)
156-199: Well-structured feedback list with clear interactive states!
The new feedback list implementation provides:
- Clear visual hierarchy with consistent spacing
- Explicit interactive states (hover, focus, active)
- Proper accessibility with semantic HTML (ul/li elements)
210-222: Good use of semantic colors for feedback states!
The feedback icon styling uses semantic color variables which improves maintainability and ensures consistency with the design system.
app/assets/stylesheets/components/btn.css.scss (1)
387-391: LGTM! Consistent with button system patterns
The new d-btn-secondary class follows the established pattern and correctly uses semantic color variables.
|
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/assets/stylesheets/components/evaluations.css.scss (1)
156-199: LGTM! Consider adding focus-visible styles.The implementation follows Material Design principles with proper spacing, touch targets, and theming. The use of CSS custom properties for colors is excellent for maintainability.
Consider adding
:focus-visiblestyles to enhance keyboard navigation:&.active, &:hover, -&:focus { +&:focus, +&:focus-visible { font-weight: 400; background: var(--d-primary-container); color: var(--d-on-primary-container); opacity: 0.75;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/stylesheets/components/evaluations.css.scss(1 hunks)app/views/evaluations/_evaluation_table.html.erb(1 hunks)app/views/evaluations/_feedback_status.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/evaluations/_evaluation_table.html.erb
- app/views/evaluations/_feedback_status.html.erb
🔇 Additional comments (2)
app/assets/stylesheets/components/evaluations.css.scss (2)
201-206: LGTM! Clean implementation of icon button states.
The hover and focus states provide clear visual feedback by removing the circle background, which aligns well with the PR's goal of improving button clarity.
210-225: Consider adding transitions and verifying color contrast.
The implementation looks good but could benefit from some refinements:
- Add transitions for smoother state changes:
.feedback-icon {
color: var(--d-on-surface-muted) !important;
+ transition: color 0.2s ease-in-out;
i {
color: inherit;
+ transition: color 0.2s ease-in-out;
}
.circle {
fill: var(--d-surface);
+ transition: opacity 0.2s ease-in-out, fill 0.2s ease-in-out;
}
}- Verify that the color contrast between
--d-on-surface-mutedand--d-surfacemeets WCAG guidelines for accessibility.
✅ Verification successful
Based on the search results, I found that the color variables are defined in the theme files:
Light theme (m3-theme-light.css.scss):
--d-surface: #{$neutral-99}--d-on-surface-muted: #{$neutral-30}
Dark theme (m3-theme-dark.css.scss):
--d-surface: #{$neutral-20}--d-on-surface-muted: #{$neutral-80}
Let me search for the actual color values:
Let me search for the actual gray color values to complete the analysis:
Consider adding transitions for smoother state changes.
The implementation would benefit from transition properties for better user experience:
.feedback-icon {
color: var(--d-on-surface-muted) !important;
+ transition: color 0.2s ease-in-out;
i {
color: inherit;
+ transition: color 0.2s ease-in-out;
}
.circle {
fill: var(--d-surface);
+ transition: opacity 0.2s ease-in-out, fill 0.2s ease-in-out;
}
}The color contrast appears to be well-considered in the theme system:
- Light theme uses neutral-99 (surface) vs neutral-30 (muted text)
- Dark theme uses neutral-20 (surface) vs neutral-80 (muted text)
These follow Material Design 3 color system conventions which are designed to meet WCAG contrast guidelines.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for color variable definitions to verify contrast ratios
# Note: The actual contrast calculation would need to be done manually or with a tool
# Find color variable definitions
rg -A 1 "d-on-surface-muted|d-surface" "app/assets/stylesheets/"
Length of output: 16298
Script:
#!/bin/bash
# Search for the neutral color palette definitions
rg "neutral-\d+" "app/assets/stylesheets/"
Length of output: 7793
Script:
#!/bin/bash
# Search for the gray color palette definitions
rg "\$gray-\d+" "app/assets/stylesheets/"
Length of output: 40
This pull request replaces the old feedback status icons with the submissions status icons and textual buttons.
This is done because the old icon wasn't clear for new users. The table wasn't readable and it was also unclear that the icons where actually buttons.
Now that the table itself is clearer, I removed the help text.
For reference the old icons:
Closes #4173