Repository navigation
CHARTS-287: Storybook: Remove the admin color override - #53340
adamwoodnz wants to merge 1 commit into
Conversation
|
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! |
This comment has been minimized.
This comment has been minimized.
|
@claude please review this PR. Get the change set with a single |
This comment has been minimized.
This comment has been minimized.
|
On Claude's non-blocking notes:
|
|
Review cycle summary
Merge order: merge #53004 first. GitHub then retargets this PR to Status: |
91c1c95 to
5ae6d4e
Compare
5ae6d4e to
ace4253
Compare
LiamSarsfield
left a comment
There was a problem hiding this comment.
LGTM, I checked the CSS from the pinned packages, and all seven --wp-admin-* values on :root come out the same with and without the override. With trunk's versions it drops back to #007cba, so the change is neutral once #53004 is in.
Regarding the rebasing:
- Renovate rebased #53004 this afternoon, but this branch still has the old copy, so it shows as conflicting and the Files tab has the whole bump in it.
- Rebasing onto the current renovate/wordpress-monorepo should go through cleanly. The description also links a private claude.ai session, which probably shouldn't be on a public PR.
The :root override existed because block-editor and block-library reset --wp-admin-theme-color to the legacy #007cba. base-styles 13.3.0 (WordPress/gutenberg#83749) defaults it to #3858e9, and the block-editor 18.1.0 and block-library 11.2.0 builds carry that default, so the override no longer changes anything.
8247c2b to
bfd0e9e
Compare
|
Thanks for checking the CSS against both sets of versions.
|
Fixes https://linear.app/a8c/issue/CHARTS-287
Why
No visible change in Storybook. Stories already render with the modern admin color (blueberry). The WordPress packages now give that color by default, so the workaround that forced it is dead code that would hide a regression if the default changed again.
Important
Stacked on #53004 (Renovate: Update wordpress monorepo), which bumps storybook to
@wordpress/base-styles13.3.0,block-editor18.1.0 andblock-library11.2.0. Without that bump, removing the override brings back the legacy#007cba. Merge #53004 first; this PR then rebases ontotrunk.Proposed changes
:root { @include mixins.admin-scheme(#3858e9); }override, its comment, and the now-unused@wordpress/base-styles/mixinsimport fromprojects/js-packages/storybook/storybook/style.scss.js-packages/storybookchangelog entry.Related product discussion/links
@wordpress/base-styles13.3.0.Does this pull request change what data or activity we track or use?
No.
Testing instructions
pnpm install, thenpnpm --dir projects/js-packages/storybook storybook:dev.getComputedStyle(document.documentElement).getPropertyValue('--wp-admin-theme-color')is#3858e9.adminColorSchemecontrol tomidnightorlight: the chart scope ([data-testid=charts-scope]) resolves to#cf4339or#007cba.themeNametocustomwith an accent color: the chart scope resolves to that accent.Verification
Ran locally with Playwright against the Storybook from this branch:
:root#3858e9#3858e9#3858e9#3858e9adminColorScheme: midnight#3858e9#cf4339adminColorScheme: light#3858e9#007cbathemeName: custom, accent#ff0000#3858e9#f00stylelintpasses on the changed file. No screenshots, because the rendered output is the same as before.🤖 Generated with Claude Code