Sb filter button styling - #222
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new mobile toggle styling needs an equivalent keyboard focus-visible state for accessibility, and the lockfile dependency bump should be clarified as intentional or split/reverted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves the mobile usability of the “filters” toggle in the directory and search UI by applying UC Davis brand button styling to the mobile-collapse toggle area.
Changes:
- Adds dark-blue background styling (and hover color transition) to the mobile filter toggle in search and directory filter components.
- Removes a leftover commented-out
console.logline in the directory filters component. - Updates
@ucd-lib/theme-elementsin the directory publicpackage-lock.json(3.5.0 → 3.5.3).
File summaries
| File | Description |
|---|---|
| ucdlib-search/src/public/lib/elements/ucdlib-search-filter.tpl.js | Adds .toggle-button / .panel__title styling for the mobile filters toggle in search. |
| ucdlib-directory/src/public/src/elements/ucdlib-directory-filters.tpl.js | Adds .toggle-button / .panel__title styling for the mobile filters toggle in directory. |
| ucdlib-directory/src/public/src/elements/ucdlib-directory-filters.js | Removes commented debug output. |
| ucdlib-directory/src/public/package-lock.json | Bumps @ucd-lib/theme-elements lockfile resolution to 3.5.3. |
Review details
Files not reviewed (1)
- ucdlib-directory/src/public/package-lock.json: Generated file
Suppressed comments (1)
ucdlib-directory/src/public/package-lock.json:67
@ucd-lib/theme-elementsis being bumped in the lockfile (3.5.0 → 3.5.3), but the PR description is focused on mobile filter button styling. Lockfile-only dependency bumps can introduce unrelated behavior changes and make this PR harder to reason about.
If this upgrade is intentional, please call it out in the PR description (or split it into a separate PR). If it’s accidental (e.g., from an npm install), revert the lockfile change.
- Files reviewed: 3/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .toggle-button { | ||
| text-align: center; | ||
| background-color: #022851; | ||
| margin: 0 .75rem; | ||
| transition: background-color 0.3s ease; | ||
| } | ||
|
|
||
| .panel__title { | ||
| color: #fff; | ||
| } | ||
|
|
||
| .toggle-button:hover { | ||
| background-color: #13639e; | ||
| } |
spelkey-ucd
left a comment
spelkey-ucd
left a comment
There was a problem hiding this comment.
While you are modifying the files, please take the opportunity to improve accessibility:
- Use an actual button element. Using native buttons is always better than
role=button - Use the existing
btn--altclass instead of making your own styles aria-expandedshould be accompanied byaria-controls
For mobile viewers, the filters button for both the directory filters and the search filters are not very accessible to view. This changes to a standard uc davis dark blue color button functionality when in the mobile view.
I changed the mobile filters css to have background color of blue and hover function of light blue since it has the toggle button class already.
May be different screen sizes not accounted for but I tested to make sure the transition is smooth. No security implications.
I will confirm it works by checking that it appears with screen size change as well as still functions correctly as a toggle.