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 table 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?
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