Skip to content

Add reduce motion check on animated counts#7924

Merged
gilluminate merged 4 commits intomainfrom
gill/reduce-motion-counts
Apr 16, 2026
Merged

Add reduce motion check on animated counts#7924
gilluminate merged 4 commits intomainfrom
gill/reduce-motion-counts

Conversation

@gilluminate
Copy link
Copy Markdown
Contributor

simply adds a check for whether the user prefers reduced motion when doing the animated counts on the dashboard.

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Apr 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
fides-plus-nightly Ready Ready Preview, Comment Apr 16, 2026 7:42pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
fides-privacy-center Ignored Ignored Apr 16, 2026 7:42pm

Request Review

@gilluminate gilluminate requested a review from kruulik April 14, 2026 21:32
@gilluminate gilluminate marked this pull request as ready for review April 14, 2026 21:32
@gilluminate gilluminate requested a review from a team as a code owner April 14, 2026 21:32
@vercel vercel Bot temporarily deployed to Preview – fides-plus-nightly April 14, 2026 21:33 Inactive
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Add reduce motion check on animated counts

The change is small, focused, and directionally correct — respecting prefers-reduced-motion is a meaningful accessibility improvement. The logic is sound: reduceMotion is properly added to the dependency array, which means the effect re-runs correctly if the user changes their OS preference at runtime (since usePrefersReducedMotion subscribes to MediaQueryList.change events).

Must Fix

return setValue(target) pattern (see inline comment on line 15)

The return setValue(target) pattern is misleading. React state setters return void, so this is equivalent to return undefined. In a useEffect callback, returning something implies a cleanup function — this looks like it's returning one but isn't. Should be:

setValue(target);
return;

Suggestions

No test coverage for the new branch

There's no test file for useCountUp. The new reduceMotion path is the most important case to guard against regression — specifically that setValue(target) fires immediately and no requestAnimationFrame is scheduled. A renderHook test mocking window.matchMedia to return matches: true would cover this clearly.

First-render flash (pre-existing, but worth noting while touching the hook)

usePrefersReducedMotion initializes to false before the useEffect fires. On the first render, reduceMotion is always false, so the animation ticks for at least one frame before the hook snaps to the final value for users with reduced motion enabled. A common fix is to initialize synchronously if window is available:

const [prefersReducedMotion, setPrefersReducedMotion] = useState(
  () => typeof window !== "undefined"
    ? window.matchMedia("(prefers-reduced-motion: reduce)").matches
    : false
);

This is pre-existing behavior in usePrefersReducedMotion, not introduced by this PR — flagging it as an informational note since the hook is being touched anyway.


🔬 Codegraph: connected (46711 nodes)


💡 Write /code-review in a comment to re-run this review.

const startRef = useRef<number>();
const reduceMotion = usePrefersReducedMotion();

useEffect(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clients/admin-ui/src/home/useCountUp.ts:15

return setValue(target) is misleading here. setValue is a React state setter that returns void. In a useEffect callback, the only meaningful return value is a cleanup function — returning the result of a side-effect call looks like it's returning a cleanup, but it's actually returning undefined.

Use the idiomatic form instead:

if (reduceMotion) {
  setValue(target);
  return;
}

This makes the intent explicit: set state, then exit early.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 14, 2026

Title Lines Statements Branches Functions
admin-ui Coverage: 8%
6.18% (2709/43817) 5.38% (1340/24866) 4.24% (550/12959)
fides-js Coverage: 78%
78.98% (1962/2484) 65.55% (1214/1852) 72.57% (336/463)
privacy-center Coverage: 88%
85.97% (331/385) 81.36% (179/220) 78.87% (56/71)

@kruulik
Copy link
Copy Markdown
Contributor

kruulik commented Apr 16, 2026

@gilluminate Could you add this to the useChartAnimation in clients/fidesui/src/components/charts/chart-utils.ts?

With prefers-reduced-motion: reduce active, the hook should return false, disabling animations across all 6 Recharts chart components.

Move usePrefersReducedMotion from admin-ui to fidesui so useChartAnimation
can disable Recharts animations when prefers-reduced-motion is active.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@gilluminate
Copy link
Copy Markdown
Contributor Author

@gilluminate Could you add this to the useChartAnimation in clients/fidesui/src/components/charts/chart-utils.ts?

Done!

Copy link
Copy Markdown
Contributor

@kruulik kruulik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@gilluminate gilluminate enabled auto-merge April 16, 2026 19:35
@gilluminate gilluminate disabled auto-merge April 16, 2026 19:35
@gilluminate gilluminate enabled auto-merge April 16, 2026 19:38
@gilluminate gilluminate added this pull request to the merge queue Apr 16, 2026
Merged via the queue into main with commit c49d20f Apr 16, 2026
52 of 54 checks passed
@gilluminate gilluminate deleted the gill/reduce-motion-counts branch April 16, 2026 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants