Theme: Simplify ramp object by flattening warnings to optional top-level property #72942
Conversation
aduth
added
[Type] Performance
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| { color: string; warning: boolean } | ||
| >; | ||
| const rampResults = {} as Record< keyof Ramp, string >; | ||
| let warnings: string[] | undefined; |
There was a problem hiding this comment.
warnings could always be an array, mostly empty. Adding the undefined option just forces users to do extra ?. checks.
There was a problem hiding this comment.
The idea with making it undefined is that it could be excluded from serialized output, further reducing size (see default-ramps.ts). But it's a good point that this benefit is partially offset by having to do all the subsequent optional handling in the code.
There was a problem hiding this comment.
I think I might keep this optional. I don't hold the opinion very strongly, but with the point of this pull request being to make the ramp objects more concise, this is an intentional and effective part of that.
aduth
force-pushed
the
update/theme-flatten-warnings
branch
from
ef280ab to
58eb055
Compare
|
Flaky tests detected in 58eb055. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19304365111
|
What?
Closes #72931
Updates the object shape generated by the theme package to simplify how colors and warnings are referenced from a generated color ramp.
Follow-up to #72847
Why?
How?
Rather than assigning each ramp value as an object of
{ color: string, warning: boolean }, optimistically assumes that colors will be generated without warnings and instead assigns the value asstring, with warnings only assigned as a top-level object key if any exist.Optionally, as mentioned by @jsnajdr in #72847 (comment), we could remove
warningbehavior altogether, as its primary purpose is for debugging. Personally I feel it might be valuable to keep for the short-term to protect against possible regressions in contrast matching, but this should serve as an incremental step in that direction, since it makes it easier to removewarningaltogether in assuming that it's optional and likely excluded in most cases.Testing Instructions
Validate no regressions in behavior of behavior of theme provider:
npm run storybook:startVerify that building theme package succeeds and produces no local changes:
npm run --workspace @wordpress/theme buildgit status