-
Notifications
You must be signed in to change notification settings - Fork 23
fix: correct cursor affordances on Dashboard (ID-691) #1170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
corypride
wants to merge
2
commits into
main
Choose a base branch
from
cpride/hover_cursor
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| # ID-691 — Dashboard table hover cursor should not change when there is no action | ||
|
|
||
| **For:** Sonnet (implementer) | ||
| **Source task:** Asana ID-691 "Hover cursor on Dashboard table should not change if there is no new action" | ||
| **Working dir:** `/home/cpride/code/devWork/ULWork/v2/UnlockEdv2` | ||
| **Stack:** React + TypeScript + Tailwind CSS, frontend in `frontend/` | ||
|
|
||
| --- | ||
|
|
||
| ## Problem | ||
|
|
||
| On the admin Dashboard, hovering over rows of the **Facility Health table** changes the mouse | ||
| cursor to a pointer (hand) and applies clickable-looking hover styling (the facility name | ||
| changes to the brand/accent color). This signals that the row is clickable, but the row has | ||
| **no `onClick` handler and no navigation** — nothing happens when clicked. This false affordance | ||
| is the bug shown in the attached `dashboard_cursor_err.mp4`. | ||
|
|
||
| Desired rule: **the cursor (and clickable hover affordances) should only change on hover when the | ||
| row actually has an action.** | ||
|
|
||
| ## Root cause (confirmed) | ||
|
|
||
| File: `frontend/src/pages/Dashboard.tsx` | ||
| Component: `FacilityHealthTable` (lines ~581–703) | ||
|
|
||
| The `<tr>` is rendered with `cursor-pointer` and link-colored hover text, but has **no `onClick`**: | ||
|
|
||
| ```tsx | ||
| // Dashboard.tsx ~line 634-642 | ||
| {rows.map((row) => ( | ||
| <tr | ||
| key={row.facility_id} | ||
| className="hover:bg-surface-hover/50 dark:hover:bg-[#262626]/50 cursor-pointer transition-colors" | ||
| > | ||
| <td className="px-6 py-4"> | ||
| <div className="text-brand-dark dark:text-white hover:text-brand dark:hover:text-[#8fb55e] transition-colors"> | ||
| {row.facility_name} | ||
| </div> | ||
| </td> | ||
| ... | ||
| ``` | ||
|
|
||
| Two misleading affordances: | ||
| 1. `cursor-pointer` on the `<tr>` — changes the cursor to a hand. | ||
| 2. `hover:text-brand dark:hover:text-[#8fb55e]` on the facility-name `<div>` — makes the name | ||
| look like a link on hover. | ||
|
|
||
| The neutral row background highlight (`hover:bg-surface-hover/50 ...`) is fine to keep — it only | ||
| indicates the hovered row, not clickability. | ||
|
|
||
| ## Convention in this codebase (confirms intended fix) | ||
|
|
||
| - `frontend/src/components/shared/DataTable.tsx` (~line 102-113): `onRowClick && 'cursor-pointer'` | ||
| — pointer only when a click handler exists. | ||
| - `frontend/src/pages/Dashboard.tsx` `MissingAttendanceWidget` (~line 463-480): `cursor-pointer` | ||
| only when `isDepartment` is true, which is also the only case it navigates. | ||
| - `frontend/src/components/ui/table.tsx` `TableRow` (~line 55-65): base row has hover background | ||
| but no `cursor-pointer` by default. | ||
|
|
||
| Only `FacilityHealthTable` violates this. `TodaysSchedule`, `MissingAttendanceWidget`, and | ||
| `MetricCards` already gate cursor/click correctly. | ||
|
|
||
| ## Implementation plan | ||
|
|
||
| The facility rows are not clickable today, so remove the clickable affordances. Do **not** invent | ||
| a navigation target — that is a product decision, not part of this bug. | ||
|
|
||
| ### Step 1 — `FacilityHealthTable` row className (line ~637) | ||
|
|
||
| From: | ||
| ```tsx | ||
| className="hover:bg-surface-hover/50 dark:hover:bg-[#262626]/50 cursor-pointer transition-colors" | ||
| ``` | ||
| To (drop `cursor-pointer`): | ||
| ```tsx | ||
| className="hover:bg-surface-hover/50 dark:hover:bg-[#262626]/50 transition-colors" | ||
| ``` | ||
|
|
||
| ### Step 2 — facility-name `<div>` (line ~640) | ||
|
|
||
| From: | ||
| ```tsx | ||
| <div className="text-brand-dark dark:text-white hover:text-brand dark:hover:text-[#8fb55e] transition-colors"> | ||
| {row.facility_name} | ||
| </div> | ||
| ``` | ||
| To: | ||
| ```tsx | ||
| <div className="text-brand-dark dark:text-white"> | ||
| {row.facility_name} | ||
| </div> | ||
| ``` | ||
|
|
||
| ### Step 3 — sanity scan | ||
|
|
||
| Grep `Dashboard.tsx` for any other element pairing `cursor-pointer` with no `onClick`/navigation. | ||
| The scan revealed that `FacilityHealthTable` was the only element with a *false* pointer affordance. | ||
| It also surfaced the inverse problem in `QuickActions`: two `<button>` elements that *do* navigate | ||
| but were missing `cursor-pointer` due to Tailwind's Preflight resetting `<button>` to | ||
| `cursor: default`. | ||
|
|
||
| ### Step 4 — QuickActions buttons (lines ~551, 566) | ||
|
|
||
| `QuickActions` contains two buttons that navigate via `onClick`: | ||
|
|
||
| - **View All Programs** → `navigate('/programs')` | ||
| - **Browse Classes** → `navigate('/classes')` | ||
|
|
||
| Both were missing `cursor-pointer`. Add it to each: | ||
|
|
||
| ```tsx | ||
| // before | ||
| className="... transition-colors" | ||
|
|
||
| // after | ||
| className="... transition-colors cursor-pointer" | ||
| ``` | ||
|
|
||
| ## Verification | ||
|
|
||
| 1. `cd frontend && npm run typecheck` and `npm run lint` — no new errors (className-only changes). | ||
| 2. Run the app, open the admin Dashboard: | ||
| - Hover Facility Health rows: cursor stays the default arrow; facility name no longer changes color. | ||
| - Hover **View All Programs** and **Browse Classes** buttons: cursor changes to pointer. | ||
| - Subtle row background highlight on hover is acceptable to keep. | ||
| 3. Confirm against `dashboard_cursor_err.mp4` that the reported behavior no longer occurs. | ||
|
|
||
| ## Out of scope / notes | ||
|
|
||
| - This fix intentionally does **not** add click navigation to facility rows. If the team later | ||
| decides facility rows should navigate (e.g., to a facility detail view), the correct change is | ||
| to **add** an `onClick`/navigation handler — at which point `cursor-pointer` and the | ||
| link-colored name become correct affordances again. Separate feature, not this bug. | ||
| - Keep changes minimal, match surrounding Tailwind style, no new deps. | ||
| - Branch off `main` (e.g. `fix/dashboard-hover-cursor`); commit message references ID-691. | ||
| Include a short before/after screen capture in the PR since it's a visual change. | ||
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.