Charts: tokenize shadow & animation values with WPDS elevation/motion tokens [CHARTS-200]#49947
Charts: tokenize shadow & animation values with WPDS elevation/motion tokens [CHARTS-200]#49947adamwoodnz wants to merge 6 commits into
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
|
@copilot review this PR. Leave your feedback as review comments only — do NOT push commits, apply suggestions, or modify the branch in any way. I will make all code changes myself. |
The tooltip, line-chart popover, zoom-reset button, and conversion-funnel tooltip used hand-rolled rgba shadows that couldn't follow the design system. Route them through --wpds-elevation-sm so elevation is themeable, with the WPDS spec value as the fallback (per the package's fallback-matches-spec rule) rather than the old approximations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuxxjRfTNYRtfYak8CzgzK
Hover/state transitions in the leaderboard, conversion-funnel, and legend hard- coded their durations (0.15s/0.2s/0.3s) and easing. Map them to --wpds-motion-duration-sm/md/lg and --wpds-motion-easing-subtle so motion follows the design system, with WPDS spec values as fallbacks. The prefers-reduced-motion override that disables these transitions is left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuxxjRfTNYRtfYak8CzgzK
The bar/area/line rise and bar/leaderboard/funnel stretch entrance animations hard-coded `1s ease-out`. Route their duration and easing through --wpds-motion-duration-xl and --wpds-motion-easing-expressive so the choreography is themeable. WPDS motion caps at 400ms, so the fallback (which must match the spec value) shortens these reveals from 1s to 400ms — a deliberate move to the design system's motion scale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuxxjRfTNYRtfYak8CzgzK
) Mechanical-only: stylelint --fix reflows the var()-wrapped box-shadow, transition, and animation declarations from the prior three commits to the project's whitespace conventions (single-line shadows, multi-line motion declarations). No value changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuxxjRfTNYRtfYak8CzgzK
…-200) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuxxjRfTNYRtfYak8CzgzK
c64b643 to
328ed4a
Compare
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
This comment was marked as resolved.
This comment was marked as resolved.
Address review: the zoom-reset is a stationary control rendered inside the chart's bounding box, not a floating tooltip, so elevation-xs is the better semantic tier than elevation-sm. xs is also the most subtle WPDS shadow, which stays closest to the control's original light 0 1px 2px rgba(0,0,0,0.08). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuxxjRfTNYRtfYak8CzgzK
|
Thanks for the thorough review — addressed below. Zoom-reset button elevation tier — Good catch, agreed. It's a stationary control inside the chart, not a floating tooltip, and Fallback-mode timing/easing shifts not documented — Fair. Added a "Fallback-mode timing/easing shifts" bullet to the PR description calling out the leaderboard hover Base-tooltip Changelog No functional/security concerns flagged — thanks. Re-requesting review on the updated branch. |
|
@copilot review this PR. Leave your feedback as review comments only — do NOT push commits, apply suggestions, or modify the branch in any way. I will make all code changes myself. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@claude please review this PR. Context: Copilot has completed its rounds (LGTM after addressing the zoom-reset elevation tier). One CI check, WordPress.com Tests, is red — but it passes on every other current open PR, the change is CSS-only token substitution (all local + GitHub JS/lint/changelog checks green), and its log is on internal infra not reachable via |
This comment was marked as resolved.
This comment was marked as resolved.
|
Thanks @claude — both final-check items verified, no code changes needed: 1. Fallbacks vs WPDS source of truth — cross-checked all changed fallbacks against the installed
2. Reduced-motion + the faster reveals — confirmed suppressed. The [ styles[ `bar-chart--animated${ horizontal ? '-horizontal' : '' }` ] ]:
animation && ! prefersReducedMotion,Every animated chart (bar/area/line/leaderboard/funnel) imports WordPress.com Tests — agreed it needs a maintainer with sandbox access to confirm; a pure SCSS fallback swap has no path to a PHP/wpcom failure. Leaving it flagged for the human reviewer. |
This comment was marked as resolved.
This comment was marked as resolved.
Review cycle summaryCopilot (2 rounds): Round 1 raised 5 points — addressed the actionable one (zoom-reset button → @claude (final round): LGTM with two non-blocking checks, both verified —
CI: All GitHub-API-visible checks green (build, JS/PHP tests, lint, E2E, changelogger). WordPress.com Tests is red — it runs on a8c-internal infra not inspectable or rerunnable via Status: Ready for human merge review. No unaddressed human-reviewer comments; no open inline threads. Sole caveat: the internal WordPress.com Tests check above. |
Why
The charts package is migrating to WordPress UI + Theme (WPDS), and shadows/motion were the last fully-hardcoded style category — 0% tokenized. Box-shadows were hand-rolled
rgba()values and transitions/animations used literal timings (0.15s/0.2s/0.3s,1s ease-out), so neither could follow the design system or be themed. WPDS now ships both--wpds-elevation-*and--wpds-motion-*tokens, so these can finally be routed through the design system the same way the package's spacing, typography, and border values already are.Proposed changes
--wpds-elevation-sm— base tooltip, line-chart annotation popover, and conversion-funnel tooltip. All three are floating contextual surfaces, which map toelevation-sm("Tooltips, Snackbar").--wpds-elevation-xs— zoom-reset button. It's a stationary control rendered inside the chart's bounding box, not a floating overlay, soelevation-xsis the correct semantic tier and stays closest to its original subtlergba(0,0,0,0.08)shadow.--wpds-motion-duration-sm/md/lg+--wpds-motion-easing-subtle— leaderboard, conversion-funnel, and legend hover/state transitions. Theprefers-reduced-motionoverride that disables these is left intact.--wpds-motion-duration-xl(400ms) +--wpds-motion-easing-expressive— bar/area/lineriseand bar/leaderboard/funnelstretchentrance animations.Per the package's fallback-matches-spec rule (
AGENTS.md), every fallback uses the exact WPDS spec value, so this adopts the design-system values rather than preserving the old pixels. Intentional visible consequences:elevation-smshadow instead of the old singlergba(0,0,0,0.1). The zoom-reset button useselevation-xsinstead — it's a stationary control, not a floating tooltip, and xs stays closest to its original subtle0.08shadow.0.15s → 100ms; the loading-state fade goes0.3s → 300ms; andease/ease-in-outare unified toeasing-subtle(cubic-bezier(0.15, 0, 0.15, 1)). These are intentional consequences of adopting the DS motion scale, not regressions.1sto400ms— WPDS motion caps at 400ms, so the reveals move onto the design system's motion scale (a ~2.5× speed-up). This was a deliberate call rather than introducing charts-scoped motion tokens.Commits are split by concern (shadows / transitions / animations / formatting / changelog) for easy review.
Screenshots
Rendered from the package's real compiled CSS with the WPDS tokens loaded. Both tooltips show the layered
--wpds-elevation-smshadow; the bars use the--wpds-motion-duration-xl(400ms) +easing-expressivereveal.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
pnpm --filter @automattic/charts build(orjetpack build js-packages/charts).pnpm --filter @automattic/charts test(913 tests).elevation-smshadow.elevation-xsshadow.expressiveeasing (snappier than before).@wordpress/themeThemeProviderthe shadow/motion follow the theme; without one, the fallbacks (which equal the WPDS spec values) render identically.