CTS - MyAssessment UI changes#155
Open
rlorenzo wants to merge 2 commits into
Open
Conversation
rlorenzo
force-pushed
the
VPR-104-accessibility-audit-CTS-myassessments
branch
from
9db50f7 to
daef548
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
==========================================
+ Coverage 42.96% 42.97% +0.01%
==========================================
Files 877 878 +1
Lines 51468 51483 +15
Branches 4802 4807 +5
==========================================
+ Hits 22113 22125 +12
- Misses 28831 28832 +1
- Partials 524 526 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Bundle ReportChanges will increase total bundle size by 16.95kB (0.79%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
|
rlorenzo
force-pushed
the
VPR-104-accessibility-audit-CTS-myassessments
branch
from
daef548 to
6057b7e
Compare
Contributor
📝 WalkthroughWalkthroughAssessmentBubble switched to computed-driven state and a conditional button/span template; CSS adds bubble and level chip styles. MyAssessments gains displayMode/timeline support and uses level chips. New EpaProgressionChart and EpaVoiceThread components render interactive timelines and gutters. Tests and a Vitest lint override were added. ChangesAssessmentBubble Component & Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@VueApp/src/CTS/__tests__/assessment-bubble.test.ts`:
- Around line 44-54: Add a new test in assessment-bubble.test.ts that covers the
accessibility edge case where levelName is empty or omitted: use createWrapper
with maxValue and value (e.g., maxValue: 5, value: 2) and set levelName: "" (and
optionally another case with levelName undefined) and assert that the
span[role="img"] aria-label is not an empty string and instead contains the
numeric fallback (match /\b2 of 5\b/i); name the test to describe the edge case
(e.g., "falls back to numeric label when levelName is empty") and mirror the
style of the existing tests so it fails if component produces aria-label="".
In `@VueApp/src/CTS/assets/cts.css`:
- Around line 96-119: Duplicate color definitions exist between
.assessmentBubble5_1–5 and .levelChip--1–5; consolidate by extracting the five
color pairs into CSS custom properties (e.g. --level-bg-1..5 and
--level-fg-1..5) and create shared modifier classes (e.g. .levelPalette--1..5)
that set background-color and color via those vars, then update
.assessmentBubble5_1..5 and .levelChip--1..5 to simply include the corresponding
shared modifier class (or reference the variables) so palette changes are
managed in one place; modify references in the stylesheet for the selectors
assessmentBubble5_* and levelChip--* accordingly.
In `@VueApp/src/CTS/components/AssessmentBubble.vue`:
- Line 27: The emit definition is untyped so TypeScript won't enforce that
"bubble-click" carries a numeric id; replace the untyped defineEmits in
AssessmentBubble.vue with a typed signature (e.g. use defineEmits<{ (e:
'bubble-click', id: number): void }>() or defineEmits<{ 'bubble-click': (id:
number) => void }>() ) so emitting "bubble-click" requires a number, and ensure
places that call emit("bubble-click", ...) still pass a number (matching
MyAssessments.vue's handleAssessmentClick(id: number)).
- Around line 64-69: The span with role="img" in AssessmentBubble.vue uses the
prop levelName which defaults to "" and thus renders aria-label="" (invalid per
WCAG); change the template so aria-label is only bound when levelName is
non-empty and set aria-hidden to true when levelName is falsy (e.g.
:aria-label="levelName || undefined" and :aria-hidden="!levelName") so the
element is treated as decorative when no name is provided; update bindings on
the <span> that uses role="img" and the levelName prop accordingly.
- Around line 48-63: Replace the raw <button> with a Quasar q-btn: keep the
v-if="id", type="button" (if desired), :aria-label using the same levelName
expression, `@click`="clickBubble", and add flat, padding="none", round so it
matches the reset; render the visual bubble inside q-btn (e.g., as the default
slot using the existing <span :class="['assessmentBubble', bubbleClass]"
aria-hidden="true" />) and keep the q-tooltip with its content and {{ props.text
}}; after changing to q-btn, remove the custom .assessmentBubbleTrigger reset
rule and the :focus-visible override from cts.css so Quasar’s built-in
focus/hover/ripple behavior is used instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca3eb28b-1ab4-462c-b6dd-1d6df9777221
📒 Files selected for processing (4)
VueApp/src/CTS/__tests__/assessment-bubble.test.tsVueApp/src/CTS/assets/cts.cssVueApp/src/CTS/components/AssessmentBubble.vueVueApp/src/CTS/pages/MyAssessments.vue
rlorenzo
force-pushed
the
VPR-104-accessibility-audit-CTS-myassessments
branch
from
6057b7e to
43056bc
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Author
|
@bsedwards New changes to the timeline display. Example: https://secure-test.vetmed.ucdavis.edu/2/CTS/MyAssessments?student=19774
|
rlorenzo
force-pushed
the
VPR-104-accessibility-audit-CTS-myassessments
branch
from
b96e286 to
e2bc753
Compare
rlorenzo
force-pushed
the
VPR-104-accessibility-audit-CTS-myassessments
branch
from
f0393ce to
556a9f0
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
VueApp/src/CTS/components/AssessmentBubble.vue (1)
52-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaw
<button>violates the guideline; replace withq-btn.The template uses a plain
<button>element. As per coding guidelines, Quasar components must be used instead of HTML elements.♻️ Replace with q-btn
- <button + <q-btn v-if="id !== undefined" - type="button" - class="assessmentBubbleTrigger" + flat + round + dense + padding="none" :aria-label="levelName ? `${levelName}, open assessment details` : 'Open assessment details'" `@click`="clickBubble" > <span :class="bubbleClasses" aria-hidden="true" /> <q-tooltip class="text-body2"> <div><strong>Click to open details</strong></div> <div class="assessmentBubbleTooltipText">{{ props.text }}</div> </q-tooltip> - </button> + </q-btn>With
q-btn, the.assessmentBubbleTriggerreset rule incts.css(lines 60–68) and the:focus-visibleoverride (lines 78–82) can be removed.As per coding guidelines: "Use Quasar components (
q-btn,q-dialog,q-banner, etc.) instead of HTML elements."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@VueApp/src/CTS/components/AssessmentBubble.vue` around lines 52 - 67, Replace the raw <button> in AssessmentBubble.vue with a Quasar q-btn while preserving behavior: use v-if="id !== undefined", set flat/fab/round/size or attributes to match the visual reset previously provided by .assessmentBubbleTrigger, bind :aria-label="levelName ? `${levelName}, open assessment details` : 'Open assessment details'", attach `@click`="clickBubble", and render the same inner visual element by placing the span with :class="bubbleClasses" and aria-hidden="true" inside the q-btn (or use q-btn's default slot); keep the q-tooltip as the q-btn child. After replacing the element, remove the .assessmentBubbleTrigger reset rule and the :focus-visible override from cts.css as noted. Ensure props.text is still used inside the tooltip and that clickBubble and bubbleClasses remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@VueApp/src/CTS/assets/cts.css`:
- Around line 96-119: Extract the repeated color palette into CSS custom
properties and reference them from the three class families so updates are
centralized: define variables (e.g., --palette-1-bg, --palette-1-fg ... through
palette-5) at a root or component scope, then replace the background-color/color
declarations in .assessmentBubble5_1..5, .levelChip--* and .cts-dot-* to use
those variables (e.g., background-color: var(--palette-1-bg); color:
var(--palette-1-fg)). Update any selectors that map to the same palette entries
to reference the appropriate variable names so one change to the variables
updates all three families. Ensure variable names are unique and descriptive and
scoped where needed ( :root or .cts ) to avoid collisions.
In `@VueApp/src/CTS/components/EpaProgressionChart.vue`:
- Around line 72-79: Replace the inline literals in the chart padding logic with
named constants to make intent clear: extract 86_400_000 into a constant like
DAY_MS, the 7 into PAD_DAYS (or DAYS_PADDING), and 0.05 into PAD_RATIO (or
PADDING_RATIO), then use those constants in the block that sets min/max and the
else branch; update the code around the function that adjusts min/max (the block
using min, max, and dayMs) so it references DAY_MS, PAD_DAYS, and PAD_RATIO
instead of the raw numbers.
- Line 129: Extract the magic number 2.4 into a clearly named constant (e.g.,
DOT_SPREAD_FACTOR or DOT_SPREAD_MULTIPLIER) and use it in the calculation for
step (const step = dotRadius.value * DOT_SPREAD_FACTOR) so the layout tuning
parameter is explicit and easily adjustable; add the new constant near the other
layout constants or component setup and update any related comments or usages
referencing step/dotRadius to reflect the extracted constant.
- Line 162: Extract the magic number 0.3 used to compute bulge into a named
constant (e.g., QUADRATIC_BULGE or BULGE_SCALE) and replace the literal in the
computation of bulge (currently const bulge = geometry.value.LANE_H * 0.3) with
that constant (const bulge = geometry.value.LANE_H * QUADRATIC_BULGE); declare
the constant near the component setup or top of the script so its purpose is
clear and document its role in one short comment; ensure references to
geometry.value.LANE_H and the bulge variable remain unchanged.
In `@VueApp/src/CTS/components/EpaVoiceThread.vue`:
- Around line 54-56: The magic numbers in lvX should be replaced with named
constants to clarify the gutter layout: extract the values 6 and 5 into
descriptively named constants (e.g., GUTTER_BASE and GUTTER_STEP or
GUTTER_OFFSET and GUTTER_INCREMENT) and update function lvX(lv: number): number
{ return GUTTER_BASE + (lv - 1) * GUTTER_STEP } so callers of lvX and future
maintainers see the layout intent; declare the constants near the top of the
component/file so lvX and related SVG coordinate logic reference the named
values.
- Line 77: Extract the magic number 18 used in y: el.getBoundingClientRect().top
- gutterTop + 18 into a named constant (e.g., GUTTER_DOT_VERTICAL_OFFSET = 18)
declared near the top of the component script (or in the constants section) and
replace the literal in the expression with that constant to document the layout
contract; update any other occurrences that align gutter dots with card centers
to use the same constant to keep behavior consistent.
---
Duplicate comments:
In `@VueApp/src/CTS/components/AssessmentBubble.vue`:
- Around line 52-67: Replace the raw <button> in AssessmentBubble.vue with a
Quasar q-btn while preserving behavior: use v-if="id !== undefined", set
flat/fab/round/size or attributes to match the visual reset previously provided
by .assessmentBubbleTrigger, bind :aria-label="levelName ? `${levelName}, open
assessment details` : 'Open assessment details'", attach `@click`="clickBubble",
and render the same inner visual element by placing the span with
:class="bubbleClasses" and aria-hidden="true" inside the q-btn (or use q-btn's
default slot); keep the q-tooltip as the q-btn child. After replacing the
element, remove the .assessmentBubbleTrigger reset rule and the :focus-visible
override from cts.css as noted. Ensure props.text is still used inside the
tooltip and that clickBubble and bubbleClasses remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e987c16-3c02-4637-b0ab-d1e75bc43d50
📒 Files selected for processing (7)
.oxlintrc.jsonVueApp/src/CTS/__tests__/assessment-bubble.test.tsVueApp/src/CTS/assets/cts.cssVueApp/src/CTS/components/AssessmentBubble.vueVueApp/src/CTS/components/EpaProgressionChart.vueVueApp/src/CTS/components/EpaVoiceThread.vueVueApp/src/CTS/pages/MyAssessments.vue
rlorenzo
force-pushed
the
VPR-104-accessibility-audit-CTS-myassessments
branch
from
556a9f0 to
f5161e4
Compare
- Two display modes via q-tabs: Timeline (default when a student views their own page) and Compact (default when a non-student opens ?student=X). q-tabs gives us role="tablist" + arrow-key navigation. - New Timeline view (EpaVoiceThread + EpaProgressionChart) renders each EPA as voice-card comments with a gutter spline connecting them and a chart-expand dialog. Compact mode's expanded comments use the same voice-card style so the two modes feel consistent. - AssessmentBubble: typed defineProps/defineEmits; aria-label surfaces the descriptive level name (never the numeric score); the decorative span falls back to aria-hidden when levelName is empty so role="img" never lands with an empty label. - EpaVoiceThread: ResizeObserver keeps the gutter spline aligned after window resize or font load; useId() prevents collisions on aria-labelledby when multiple instances render with null epaIds. - MyAssessments: bucket assessments by epaId in a computed Map so the template's repeated lookups don't refilter the full list per render. - Drop the unused legacy/abbreviation/blend display variants along with their supporting components, props, and CSS. - Lock the AssessmentBubble contract with 16 unit tests covering the aria-label privacy paths, value→class mapping, and click emission.
vitest plugin ships both prefer-importing-vitest-globals (wants explicit imports) and no-importing-vitest-globals (wants globals) enabled by default, so oxlint --fix flips test files between the two warnings on every run. vitest.config sets globals: true and every test file in VueApp/src uses the globals form, so disable the prefer- rule to match the established convention.
rlorenzo
force-pushed
the
VPR-104-accessibility-audit-CTS-myassessments
branch
from
f5161e4 to
2a0e2cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Final piece of the CTS accessibility audit for MyAssessments — collapses the page down to two settled designs (Compact + Timeline), keeps the privacy-preserving
AssessmentBubbleredesign, and adds the Timeline narrative view as the default for students viewing their own page.Two display modes
A
q-tabsat the top of the page switches between Compact and Timeline. The default depends on the viewer:/CTS/MyAssessmentsfor themselves (no?student=query). Renders the new narrativeEpaVoiceThreadcards./CTS/MyAssessments?student=X. Renders the compact bubble grid; expanding an EPA reveals the same voice-card style used in Timeline (quote + name·service·date·supervision-dot byline).The toggle wrapper carries
role="group"+aria-label="Assessment display mode"so screen readers announce it as a single grouped control.Compact mode (
AssessmentBubble.vue)aria-label— the rating dot reads the descriptive level name (e.g. "Trust with indirect supervision"), never the numeric value. Prevents screen readers from announcing a low rating aloud in shared spaces. (4.1.2 Name, Role, Value)rgba(62,127,238,0.3)→rgba(11,3,139,1)), 1.5rem diameter, blue (Quasar primary)focus-visiblering, 0.25rem horizontal spacing. (1.4.1 Use of Color, 1.4.11 Non-text Contrast)Timeline mode (new components)
EpaVoiceThread.vue— renders each EPA as a card with a left-gutter SVG spline (decorative,aria-hidden) connecting hover/focus-synchronized "voice" cards. Each voice shows the comment as a pull-quote and a byline with the assessor, service, date, and a colored supervision dot. Threshold-gated "show earlier" button, with a chart-expand dialog at the side.EpaProgressionChart.vue— compact + large SVG chart plotting supervision level vs. date, with same-day duplicate spread, smooth spline, and synced hover halo. Markedaria-hiddenso the voice list remains the canonical keyboard surface.Page structure (
MyAssessments.vue)<h1>for the page title;<h2>for "Entrustable Professional Activities". (1.3.1 Info and Relationships)aria-label+aria-expanded. (4.1.2)aria-labelledby+ a closeq-btnwitharia-label="Close dialog". (2.1.1 Keyboard)Accessibility audit (axe-core, WCAG 2.1 A + AA + best-practice)
Run via Playwright MCP on https://localhost:7159/CTS/MyAssessments, scoped to
<main>:Reported "incomplete" items were either positional (fixed footer overlapping a heading at a specific scroll position) or chart axis labels inside the
aria-hiddenSVG — pre-existing decorative styling, not introduced here.Tests
assessment-bubble.test.ts— 15 cases covering the privacy contract (aria-label must never contain the numeric rating), the value → class mapping, click emission, and the non-interactive span variant.Test plan
/CTS/MyAssessments(own page) defaults to Timeline/CTS/MyAssessments?student=<id>(as faculty/admin) defaults to Compactnpm run test:frontend—assessment-bubble.test.tspasses (15/15)Summary by CodeRabbit
Release Notes
New Features
Improvements