Students Tab + Enrollments Table #12211
Conversation
All images
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12211 +/- ##
==========================================
+ Coverage 66.34% 66.43% +0.09%
==========================================
Files 772 778 +6
Lines 76421 77699 +1278
Branches 7989 8020 +31
==========================================
+ Hits 50698 51622 +924
- Misses 25551 25907 +356
+ Partials 172 170 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded@reteps has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change introduces a new "Students" page for course instance administrators, featuring a fully interactive, virtualized, and sortable table of enrolled students using TanStack Table and React. It adds new React components, SQL queries, server routes, navigation tabs, download options, and accessibility enhancements, all integrated into the PrairieLearn instructor interface. Changes
Estimated code review effort4 (~90 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
apps/prairielearn/package.json (1)
63-64: Check whether@tanstack/table-coreis truly required as a runtime dependency
@tanstack/react-tablealready lists@tanstack/table-coreas a peer dependency, so installing both packages at the application level is usually unnecessary and inflates the bundle size.Please verify that:
@tanstack/react-table’s peer-dependency range is satisfied by the version you add here.- No server-side code imports
@tanstack/table-coredirectly (which would justify keeping it).If direct imports are not present, consider removing the extra entry:
- "@tanstack/react-table": "^8.21.3", - "@tanstack/table-core": "^8.21.3", + "@tanstack/react-table": "^8.21.3",apps/prairielearn/src/models/enrollment.sql (1)
17-24: Name & performance improvements for new query block
- Block name is singular (
select_course_instance_enrollment) but returns many rows – pluralising (select_course_instance_enrollments) would be clearer.SELECT *may ship unneeded columns and hinder index-only scans. Explicitly list columns you actually need.- If
course_instance_idis not already indexed, this query will require a seq-scan on large tables; consider an index:CREATE INDEX IF NOT EXISTS enrollments_course_instance_id_idx ON enrollments(course_instance_id);apps/prairielearn/src/models/enrollment.ts (1)
123-133: Consider exposing ordering or pagination
getEnrollmentsForCourseInstancereturns every row un-ordered. Large classes could generate thousands of records, causing heavy memory usage and unpredictable sort order. Expose optionallimit/offsetparameters or anORDER BY(e.g.,last_name, first_name) insideselect_course_instance_enrollment.sql.No functional blocker but worth tightening before production.
apps/prairielearn/src/pages/instructorStudents/components/RoleDescriptionModal.tsx (1)
1-50: LGTM, minor a11y noteMarkup follows Bootstrap-modal guidelines; role descriptions are clear.
For accessibility, consider addingrole="dialog"to the.modaldiv andaria-modal="true".apps/prairielearn/src/pages/instructorStudents/components/StudentDataViewMissing.tsx (1)
23-24: Hard-coded path may double-slash & missesrelIf
urlPrefixalready ends with/, the final URL will have//course_admin…. Also consider addingrel="noopener"to avoid opener hijacking.- <a href={`${urlPrefix}/course_admin/staff`}>Staff tab</a> + <a href={`${urlPrefix.replace(/\/$/, '')}/course_admin/staff`} rel="noopener"> + Staff tab + </a>apps/prairielearn/src/pages/instructorStudents/instructorStudents.html.tsx (1)
61-67: Minor accessibility / UX touchAdding
downloadmakes browsers treat the link as a file; including visually hidden text helps screen-reader users.- <a href={`${urlPrefix}/instance_admin/students/${csvFilename}`} className="btn btn-light btn-sm"> + <a + href={`${urlPrefix}/instance_admin/students/${csvFilename}`} + className="btn btn-light btn-sm" + download + >apps/prairielearn/src/pages/instructorStudents/components/StudentsTable.tsx (2)
39-46: Remove console noise before shippingThe debug
console.logstatements will fire on every header click and pollute the console in production.- // console.log('isSorted', isSorted); - if (isSorted === 'asc') { - console.log('asc'); + if (isSorted === 'asc') { return <i className="bi bi-sort-up"></i>; - } else if (isSorted === 'desc') { - console.log('desc'); + } else if (isSorted === 'desc') { return <i className="bi bi-sort-down"></i>;
74-81: Locale-dependent date formatting
toLocaleDateString()+toLocaleTimeString()depend on the client locale and may yield ambiguous formats (e.g.,MM/DD/YYYY). Consider ISO 8601 orIntl.DateTimeFormatwith an explicit locale.apps/prairielearn/src/pages/instructorStudents/instructorStudents.tsx (1)
96-118: CSV stream lacksContent-TypeMost clients rely on
text/csv. Also, set a sane cache policy.- res.attachment(req.params.filename); + res.attachment(req.params.filename); + res.type('text/csv'); + res.set('Cache-Control', 'private, max-age=0');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
apps/prairielearn/assets/scripts/esm-bundles/react-fragments/InstructorStudents.ts(1 hunks)apps/prairielearn/package.json(1 hunks)apps/prairielearn/src/components/PageLayout.html.tsx(1 hunks)apps/prairielearn/src/components/SideNav.html.ts(2 hunks)apps/prairielearn/src/lib/preact.tsx(1 hunks)apps/prairielearn/src/models/enrollment.sql(1 hunks)apps/prairielearn/src/models/enrollment.ts(2 hunks)apps/prairielearn/src/pages/instructorStudents/components/RoleDescriptionModal.tsx(1 hunks)apps/prairielearn/src/pages/instructorStudents/components/StudentDataViewMissing.tsx(1 hunks)apps/prairielearn/src/pages/instructorStudents/components/StudentsTable.tsx(1 hunks)apps/prairielearn/src/pages/instructorStudents/instructorStudents.html.tsx(1 hunks)apps/prairielearn/src/pages/instructorStudents/instructorStudents.sql(1 hunks)apps/prairielearn/src/pages/instructorStudents/instructorStudents.tsx(1 hunks)apps/prairielearn/src/server.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Core / Build (ubuntu-24.04-arm)
- GitHub Check: Native (shard 3/4)
- GitHub Check: Native (shard 1/4)
- GitHub Check: Native (shard 4/4)
- GitHub Check: Native (shard 2/4)
- GitHub Check: Docker
🔇 Additional comments (5)
apps/prairielearn/src/components/PageLayout.html.tsx (1)
145-151: Good switch toborder-end– RTL-awareReplacing
border-rightwithborder-endaligns with Bootstrap 5’s logical properties and automatically handles RTL layouts.apps/prairielearn/assets/scripts/esm-bundles/react-fragments/InstructorStudents.ts (1)
1-4: Fragment registration looks goodImport path and registration follow the existing pattern for fragments; no concerns.
apps/prairielearn/src/components/SideNav.html.ts (1)
125-132: #!/bin/bashScan package.json for FontAwesome deps to confirm the version in use
if [ -f package.json ]; then
echo "FontAwesome entries in package.json dependencies/devDependencies:"
jq '.dependencies, .devDependencies' package.json | grep -i fontawesome -C 2 || echo "No FontAwesome packages found."
else
echo "package.json not found at repo root."
fiapps/prairielearn/src/server.ts (1)
1218-1221: ```shell
#!/bin/bashLocate the instructorStudents page module
fd instructorStudents.js
Search within that file for any navSubPage assignments
rg "navSubPage" -A3 -B3 apps/prairielearn/src/pages/instructorStudents/instructorStudents.js
Search the entire codebase for navSubPage usage
rg "navSubPage"
</details> <details> <summary>apps/prairielearn/src/pages/instructorStudents/instructorStudents.html.tsx (1)</summary> `49-56`: **`courseOwners` guard can still pass `undefined`** The ternary first checks `!has_view && courseOwners ? …` but `courseOwners` is optional in props. If the router ever forgets to supply it, TypeScript won’t catch this runtime gap. Make the prop non-optional when `has_view` is false (e.g. union props) or default to `[]`. ```diff -interface InstructorStudentsContentProps { - resLocals: ResLocals; - courseOwners?: User[]; +type InstructorStudentsContentProps = + | { resLocals: ResLocals; courseOwners: User[]; csvFilename: string; students?: StudentRow[] } + | { resLocals: ResLocals; courseOwners?: undefined; csvFilename: string; students: StudentRow[] };
apps/prairielearn/src/pages/instructorStudents/instructorStudents.sql
Outdated
Show resolved
Hide resolved
apps/prairielearn/src/pages/instructorStudents/components/StudentDataViewMissing.tsx
Outdated
Show resolved
Hide resolved
apps/prairielearn/src/pages/instructorStudents/instructorStudents.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/prairielearn/src/components/SideNav.html.ts(1 hunks)apps/prairielearn/src/models/enrollment.ts(1 hunks)apps/prairielearn/src/pages/instructorStudents/instructorStudents.html.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/prairielearn/src/components/SideNav.html.ts
- apps/prairielearn/src/pages/instructorStudents/instructorStudents.html.tsx
🧰 Additional context used
🪛 GitHub Check: JavaScript
apps/prairielearn/src/models/enrollment.ts
[failure] 6-6:
'queryRows' is defined but never used
🪛 GitHub Actions: Check
apps/prairielearn/src/models/enrollment.ts
[error] 6-6: ESLint: 'queryRows' is defined but never used. (@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Native (shard 2/4)
- GitHub Check: Native (shard 3/4)
- GitHub Check: Native (shard 4/4)
- GitHub Check: Native (shard 1/4)
- GitHub Check: Docker
- GitHub Check: Core / Build (ubuntu-24.04-arm)
- GitHub Check: Core / Build (ubuntu-24.04)
apps/prairielearn/src/pages/instructorStudents/components/StudentDataViewMissing.tsx
Outdated
Show resolved
Hide resolved
apps/prairielearn/src/pages/instructorStudents/components/StudentsTable.tsx
Outdated
Show resolved
Hide resolved
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| onClearElementFocus(); | ||
| switch (e.key) { | ||
| // Support for arrow keys to move between menu items | ||
| case 'ArrowRight': | ||
| e.preventDefault(); | ||
| // Move to the next menu item | ||
| pinButtonRef.current?.focus(); | ||
| break; | ||
| case 'ArrowLeft': | ||
| e.preventDefault(); | ||
| // Move to the previous menu item | ||
| checkboxRef.current?.focus(); | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
What ARIA pattern is this built on? I tried using this with VoiceOver and it wasn't clear to me that I could/should navigate with the arrow keys and not Tab.
There was a problem hiding this comment.
https://www.w3.org/WAI/ARIA/apg/patterns/menubar/ from reading this, I felt like screenreaders expect each item in the "menu" be be focusable. I actually got this example from this context menu:
You have to use arrow keys to navigate between all the options, and only the first one is tabable.
There was a problem hiding this comment.
I can change everything to be tabbable, and no arrow keys if preferred. I'm not sure the exact pattern to use here.
There was a problem hiding this comment.
I had a question about how we are doing reviews -- do I need to re-request review in order for you to see that I have responded to your comments in this thread?
There was a problem hiding this comment.
Usually I'll see things via GitHub notification. Though now that I'm not reviewing everything, there's a higher likelihood that things get lost in the noise. Don't hesitate to ping out of band if you think I'm taking too long 🙂
There was a problem hiding this comment.
GitHub's example uses role="toolbar", which gives important context that this is actually a toolbar when read with VoiceOver. That'd indicate to a screen reader user that arrow keys would do something.
There's nothing like that in your implementation, so there's really no way for someone using a screen reader to discover that they can/should use arrow keys to navigate.
I don't know that there's really a suitable ARIA role for what you have here; from reading the description of menubar that doesn't really seem to be what this is. If you think that is in fact correct, you'll want to add role attributes as those docs specify. However, I think the simplest and most correct thing to do is just to allow everything to be tabbable.
There was a problem hiding this comment.
Done in 097261b to be tabbable.
apps/prairielearn/src/pages/instructorStudents/components/ColumnManager.tsx
Outdated
Show resolved
Hide resolved
apps/prairielearn/src/pages/instructorStudents/components/ColumnManager.tsx
Outdated
Show resolved
Hide resolved
apps/prairielearn/src/pages/instructorStudents/components/DownloadButton.tsx
Show resolved
Hide resolved
apps/prairielearn/src/pages/instructorStudents/instructorStudents.tsx
Outdated
Show resolved
Hide resolved
…entsTable.tsx Co-authored-by: Nathan Sarang-Walters <nathan@prairielearn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/prairielearn/src/pages/instructorStudents/components/ColumnManager.tsx (3)
52-89: Use consistent JSX attribute naming.The component mixes
classandclassNameattributes inconsistently. In JSX, useclassNameconsistently for all CSS class assignments.Apply this diff to use consistent naming:
- className="px-2 py-1 d-flex align-items-center justify-content-between" + className="px-2 py-1 d-flex align-items-center justify-content-between" onKeyDown={handleKeyDown} > - <label class="form-check me-auto text-nowrap d-flex align-items-stretch"> + <label className="form-check me-auto text-nowrap d-flex align-items-stretch"> <OverlayTrigger placement="top" overlay={<Tooltip>{column.getIsVisible() ? 'Hide column' : 'Show column'}</Tooltip>} @@ -65,7 +65,7 @@ <input type="checkbox" ref={checkboxRef} - class="form-check-input" + className="form-check-input" checked={column.getIsVisible()} onKeyDown={(e) => { @@ -84,7 +84,7 @@ /> </OverlayTrigger> - <span class="form-check-label ms-2" id={`${column.id}-label`}> + <span className="form-check-label ms-2" id={`${column.id}-label`}> {header} </span> </label>
90-126: Fix attribute naming and verify icon logic.Two issues to address:
- Inconsistent JSX attributes: Use
classNameinstead ofclassfor consistency.- Potentially confusing icon logic: The snowflake icon (bi-snow) appears for unpinned columns and X icon (bi-x) for pinned columns. This may be counterintuitive - users might expect the snowflake to indicate "frozen" state.
Apply this diff for consistent attribute naming:
- class="btn btn-sm btn-ghost ms-2" + className="btn btn-sm btn-ghost ms-2" aria-label={ column.getIsPinned() ? `Unfreeze '${header}' column` : `Freeze '${header}' column` } @@ -120,7 +120,7 @@ tabIndex={-1} > - <i class={`bi ${column.getIsPinned() ? 'bi-x' : 'bi-snow'}`} aria-hidden="true" /> + <i className={`bi ${column.getIsPinned() ? 'bi-x' : 'bi-snow'}`} aria-hidden="true" /> </button>Consider if the icon logic aligns with user expectations - should the snowflake represent the frozen state rather than the action to freeze?
168-246: Complete the JSX attribute consistency fixes.The component logic and accessibility are well-implemented, but the
class/classNameinconsistency needs to be addressed throughout the remaining JSX.Apply this diff to complete the consistent attribute naming:
<Dropdown.Toggle variant="outline-secondary" id="column-manager-button"> - <i class="bi bi-view-list me-2" aria-hidden="true"></i> + <i className="bi bi-view-list me-2" aria-hidden="true"></i> View </Dropdown.Toggle> <Dropdown.Menu> {pinnedColumns.length > 0 && ( <> - <div class="px-2 py-1 text-muted small" role="presentation"> + <div className="px-2 py-1 text-muted small" role="presentation"> Frozen columns </div> @@ -225,7 +225,7 @@ )} {showResetButton && ( - <div class="px-2 py-1"> + <div className="px-2 py-1"> <Button variant="secondary" size="sm" @@ -237,7 +237,7 @@ }} aria-label="Reset all columns to default visibility and pinning" > - <i class="bi bi-arrow-counterclockwise me-2" aria-hidden="true" /> + <i className="bi bi-arrow-counterclockwise me-2" aria-hidden="true" /> Reset view </Button> </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/prairielearn/src/components/PageLayout.tsx(1 hunks)apps/prairielearn/src/components/SideNav.tsx(1 hunks)apps/prairielearn/src/components/SyncErrorsAndWarnings.tsx(5 hunks)apps/prairielearn/src/lib/navPageTabs.ts(1 hunks)apps/prairielearn/src/pages/instructorStudents/components/ColumnManager.tsx(1 hunks)apps/prairielearn/src/pages/instructorStudents/components/StudentsTable.tsx(1 hunks)apps/prairielearn/src/pages/instructorStudents/instructorStudents.tsx(1 hunks)apps/prairielearn/src/pages/studentGradebook/studentGradebook.html.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/prairielearn/src/components/PageLayout.tsx
- apps/prairielearn/src/components/SyncErrorsAndWarnings.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/prairielearn/src/pages/studentGradebook/studentGradebook.html.tsx
- apps/prairielearn/src/lib/navPageTabs.ts
- apps/prairielearn/src/pages/instructorStudents/instructorStudents.tsx
- apps/prairielearn/src/pages/instructorStudents/components/StudentsTable.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Victorsss-Orz
PR: PrairieLearn/PrairieLearn#12148
File: apps/prairielearn/src/ee/lib/ai-grading/ai-grading-stats.ts:175-192
Timestamp: 2025-06-16T00:31:26.093Z
Learning: In apps/prairielearn/src/ee/lib/ai-grading/ai-grading-stats.ts, the selectGradingJobsInfo function's SQL query has been modified to ensure only one grading job per method is returned, eliminating the need for sorting to avoid arbitrary record selection with Array.prototype.find().
Learnt from: nwalters512
PR: PrairieLearn/PrairieLearn#12436
File: apps/prairielearn/src/pages/externalImageCapture/externalImageCapture.html.ts:3-3
Timestamp: 2025-07-17T14:39:21.511Z
Learning: In the PrairieLearn codebase, the migration to .tsx extensions is being done incrementally by directory. PR #12436 specifically renamed files in the `src/components/` directory from `.html.js` to `.tsx`, but files in other directories like `src/pages/` still retain their `.html.js` extensions and should continue to be imported with `.html.js` paths.
apps/prairielearn/src/components/SideNav.tsx (2)
Learnt from: nwalters512
PR: #12436
File: apps/prairielearn/src/pages/externalImageCapture/externalImageCapture.html.ts:3-3
Timestamp: 2025-07-17T14:39:21.511Z
Learning: In the PrairieLearn codebase, the migration to .tsx extensions is being done incrementally by directory. PR #12436 specifically renamed files in the src/components/ directory from .html.js to .tsx, but files in other directories like src/pages/ still retain their .html.js extensions and should continue to be imported with .html.js paths.
Learnt from: Victorsss-Orz
PR: #12148
File: apps/prairielearn/src/ee/lib/ai-grading/ai-grading-stats.ts:175-192
Timestamp: 2025-06-16T00:31:26.093Z
Learning: In apps/prairielearn/src/ee/lib/ai-grading/ai-grading-stats.ts, the selectGradingJobsInfo function's SQL query has been modified to ensure only one grading job per method is returned, eliminating the need for sorting to avoid arbitrary record selection with Array.prototype.find().
apps/prairielearn/src/pages/instructorStudents/components/ColumnManager.tsx (12)
Learnt from: jonatanschroeder
PR: #12186
File: apps/prairielearn/assets/scripts/instructorRequestCourseClient.ts:41-44
Timestamp: 2025-06-12T21:12:14.111Z
Learning: PrairieLearn uses Bootstrap 5’s static modal helpers with selector strings (e.g., bootstrap.Modal.getOrCreateInstance('#myModal')), which is valid; this should not be flagged as an error in future reviews.
Learnt from: nwalters512
PR: #10944
File: apps/prairielearn/src/components/PageLayout.html.tsx:308-315
Timestamp: 2025-03-05T20:31:32.802Z
Learning: The PrairieLearn application is taking a gradual migration approach to React/Preact, utilizing a mix of legacy HTML rendering (via a secure html tagged-template literal that performs proper escaping) and JSX components during the transition period. The temporary use of dangerouslySetInnerHTML is part of this migration strategy and will be phased out over time.
Learnt from: nwalters512
PR: #12436
File: apps/prairielearn/src/pages/externalImageCapture/externalImageCapture.html.ts:3-3
Timestamp: 2025-07-17T14:39:21.511Z
Learning: In the PrairieLearn codebase, the migration to .tsx extensions is being done incrementally by directory. PR #12436 specifically renamed files in t
1241
he src/components/ directory from .html.js to .tsx, but files in other directories like src/pages/ still retain their .html.js extensions and should continue to be imported with .html.js paths.
Learnt from: nwalters512
PR: #11528
File: apps/prairielearn/src/pages/instructorQuestionPreview/instructorQuestionPreview.ts:18-18
Timestamp: 2025-03-11T16:48:40.042Z
Learning: In the PrairieLearn codebase, TypeScript files import from .js files (e.g., import { getSearchParams } from '../../lib/url.js') even when the source file is a .ts file. This is because the code runs in Node and must point to where the transpiled files will be after compilation. This pattern is standard in TypeScript projects and explicitly supported by TypeScript compiler.
Learnt from: miguelaenlle
PR: #12093
File: apps/prairielearn/assets/scripts/question.ts:35-46
Timestamp: 2025-06-27T19:56:13.335Z
Learning: In PrairieLearn's apps/prairielearn/assets/scripts/question.ts file, vanilla JavaScript is preferred over jQuery for consistency, as the majority of the file uses vanilla JS patterns.
Learnt from: nwalters512
PR: #11932
File: apps/prairielearn/src/lib/submissions-upload.ts:113-113
Timestamp: 2025-05-07T22:05:32.431Z
Learning: In PrairieLearn's submission-upload functionality, destructive operations like deleting assessment instances are explicitly documented with JSDoc comments and UI warnings to prevent accidental data loss, especially for features intended only for development mode.
Learnt from: nwalters512
PR: #11932
File: apps/prairielearn/src/lib/submissions-upload.ts:113-113
Timestamp: 2025-05-07T22:05:32.431Z
Learning: In PrairieLearn's submission-upload functionality, destructive operations like deleting assessment instances are explicitly documented with JSDoc comments and UI warnings to prevent accidental data loss, especially for features intended only for development mode.
Learnt from: nwalters512
PR: #11425
File: apps/prairielearn/src/components/SideNavToggleButton.html.ts:5-27
Timestamp: 2025-03-16T21:29:57.477Z
Learning: In PrairieLearn's codebase, components using the html tagged template literal imported from @prairielearn/html are not React or Lit components and don't support attaching JavaScript func
F438
tions/callbacks directly to attributes. These components are primarily for templating HTML, not for handling interactivity directly.
Learnt from: nwalters512
PR: #11329
File: apps/prairielearn/src/sync/fromDisk/assessmentModules.ts:67-0
Timestamp: 2025-02-11T19:42:17.229Z
Learning: In PrairieLearn's sync utilities, interfaces like DesiredAssessmentModule intentionally only declare core properties that users must provide, while implementation details like implicit and number are managed internally by the determineOperationsForEntities() function.
Learnt from: mylesw27
PR: #12249
File: apps/prairielearn/src/components/AssessmentBadge.tsx:8-8
Timestamp: 2025-07-08T20:21:01.894Z
Learning: In PrairieLearn's JSX component architecture, there's a deliberate naming convention pattern where database functions use snake_case and JSX components use camelCase. The conversion from snake_case to camelCase happens at the JSX component level through wrapper functions (like AssessmentBadgeHtml) that serve as adapters between the database layer and the UI layer, maintaining clear separation between different layers of the application.
Learnt from: nwalters512
PR: #11842
File: apps/prairielearn/src/pages/instructorCourseAdminStaff/instructorCourseAdminStaff.html.ts:705-707
Timestamp: 2025-04-24T19:18:29.603Z
Learning: For simple tables with proper semantic structure using <thead>, <tr>, and <th> elements, the W3C Web Accessibility Initiative states that "headers do not need the scope attributes added to them as the relationship of the header with the data cells is evident by the position." However, some accessibility guidelines like WebAIM still recommend using scope for maximum clarity and compatibility with all assistive technologies.
Learnt from: nwalters512
PR: #11842
File: apps/prairielearn/src/pages/instructorCourseAdminStaff/instructorCourseAdminStaff.html.ts:705-707
Timestamp: 2025-04-24T19:12:48.176Z
Learning: For simple tables with a clear structure using <thead>, <tr>, and <th> elements, most modern screen readers can correctly infer that the headers apply to columns without needing explicit scope="col" attributes. However, including scope="col" is still considered a best practice for maximum accessibility compatibility.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Core / Build (ubuntu-24.04-arm)
- GitHub Check: Core / Build (ubuntu-24.04)
- GitHub Check: Native (shard 4/4)
- GitHub Check: Native (shard 3/4)
- GitHub Check: Native (shard 1/4)
- GitHub Check: Docker
- GitHub Check: Native (shard 2/4)
- GitHub Check: Other
- GitHub Check: JavaScript
🔇 Additional comments (4)
apps/prairielearn/src/components/SideNav.tsx (1)
130-137: LGTM! Clean implementation following established patterns.The Students tab configuration is well-implemented with appropriate permission checking, consistent URL structure, and proper icon usage. The placement after the Gradebook tab is logical for the navigation flow.
apps/prairielearn/src/pages/instructorStudents/components/ColumnManager.tsx (3)
1-14: Import structure follows established patterns correctly.The imports and type casting for react-bootstrap components are properly implemented following the patterns established in the codebase.
15-34: Well-structured component interface and logic.The interface is clearly defined and the early return pattern efficiently handles columns that don't support visibility or pinning operations. The header extraction logic properly handles different header types.
128-167: Well-structured component logic and state management.The ColumnManager component has excellent structure with proper state management, column order preservation in the pinning logic, and appropriate focus management using useEffect.
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| onClearElementFocus(); | ||
| switch (e.key) { | ||
| // Support for arrow keys to move between menu items | ||
| case 'ArrowRight': | ||
| e.preventDefault(); | ||
| // Move to the next menu item | ||
| pinButtonRef.current?.focus(); | ||
| break; | ||
| case 'ArrowLeft': | ||
| e.preventDefault(); | ||
| // Move to the previous menu item | ||
| checkboxRef.current?.focus(); | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify ARIA compliance for keyboard navigation.
The arrow key navigation only handles horizontal movement within a single menu item but doesn't follow standard menu navigation patterns. This may not align with ARIA authoring practices for dropdown menus.
Consider implementing standard menu keyboard navigation (up/down arrows between items, Enter/Space for selection) or using established ARIA patterns. The current implementation may confuse users expecting standard dropdown menu behavior.
🌐 Web query:
What are the ARIA authoring practices for dropdown menu keyboard navigation?
💡 Result:
The ARIA (Accessible Rich Internet Applications) authoring practices for dropdown menu keyboard navigation ensure that users can effectively navigate and interact with dropdown menus using a keyboard, which is crucial for accessibility.
Here are the key guidelines generally followed:
-
Tab Key: The Tab key moves focus into and out of the dropdown menu. When a dropdown menu trigger (e.g., a button) receives focus, pressing Tab moves focus to the next focusable element outside the menu.
-
Enter or Space Key: When focus is on the dropdown menu button, pressing Enter or Space opens the dropdown menu and moves focus to the first menu item.
-
Arrow Keys:
- Down Arrow: Moves focus to the next menu item. If focus is on the last item, it loops back to the first item.
- Up Arrow: Moves focus to the previous menu item. If focus is on the first item, it loops back to the last item.
-
Home and End Keys:
- Home: Moves focus to the first menu item.
- End: Moves focus to the last menu item.
-
Escape Key: Closes the dropdown menu and returns focus to the dropdown menu button.
-
Tab: When the menu is open, pressing Tab closes the menu and moves focus to the next focusable element outside the menu.
-
Focus Management: The menu items should be focusable and receive appropriate ARIA roles such as
menuitemormenuitemradio/menuitemcheckboxfor menu options that behave as radio buttons or checkboxes. -
ARIA Roles and Properties:
- The dropdown button has
aria-haspopup="true"andaria-expandedto indicate the menu state. - The menu container uses
role="menu". - Menu items use
role="menuitem"or similar roles.
- The dropdown button has
This keyboard interaction model makes dropdown menus accessible to screen reader users and keyboard-only users.
If you want, I can provide the official ARIA authoring practices documentation link or more detailed code examples. Would you like that?
Add ARIA-Compliant Keyboard Navigation to ColumnManager
The current handleKeyDown only supports left/right focus within a single menu item. According to WAI-ARIA Authoring Practices for dropdown menus, you should implement:
- ArrowDown/ArrowUp to move focus between menu items (with wrapping).
- Home/End to jump to the first/last menu item.
- Escape to close the menu and return focus to the menu button.
- Enter/Space to activate an item.
- Tab to close the menu and move focus out.
Action items:
- File:
apps/prairielearn/src/pages/instructorStudents/components/ColumnManager.tsx(lines 35–50)- Expand
handleKeyDownto handleArrowDown,ArrowUp,Home,End, andEscape. - On menu open, move focus into the first item; on close, return focus to the trigger.
- Expand
- Ensure your dropdown trigger has
aria-haspopup="true"and dynamically updatesaria-expanded. - Wrap the list with
role="menu"and give each optionrole="menuitem"(ormenuitemcheckbox/menuitemradioas needed).
Implementing these patterns will align keyboard behavior with user expectations and assistive-technology requirements.
🤖 Prompt for AI Agents
In apps/prairielearn/src/pages/instructorStudents/components/ColumnManager.tsx
around lines 35 to 50, expand the handleKeyDown function to support full
ARIA-compliant keyboard navigation by adding cases for ArrowDown and ArrowUp to
move focus between menu items with wrapping, Home and End keys to jump to the
first and last items, Escape to close the menu and return focus to the menu
button, and Enter/Space to activate the focused item. Also handle Tab to close
the menu and move focus out. Ensure the dropdown trigger element has
aria-haspopup="true" and updates aria-expanded dynamically based on menu state.
Wrap the menu list with role="menu" and assign role="menuitem" or appropriate
menuitemcheckbox/menuitemradio roles to each option. On menu open, move focus to
the first menu item; on close, return focus to the trigger button.
…rn/PrairieLearn into stengerp/tanstack-enrollments
9b38e97 to
6b853aa
Compare
Supporting changes:
Out of scope for this PR:
Closes #12200