Controls: Add maxPresetColors option to ColorControl#33998
Controls: Add maxPresetColors option to ColorControl#33998mixelburg wants to merge 6 commits intostorybookjs:nextfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduce a configurable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/addons/docs/src/blocks/controls/Color.tsx`:
- Around line 335-338: The slice branch must guard against negative,
non-integer, NaN, or Infinity values for maxPresetColors before calling
combined.slice; update the logic where maxPresetColors is used (the block
referencing combined and maxPresetColors in Color.tsx) to coerce and validate it
into a safe positive integer (e.g., if typeof maxPresetColors !== 'number' ||
!isFinite(maxPresetColors) || maxPresetColors <= 0 return combined; otherwise
use const n = Math.floor(maxPresetColors) and return combined.slice(-Math.min(n,
combined.length))). Ensure you reference and use the existing combined and
maxPresetColors identifiers when implementing the checks.
In `@code/core/src/actions/containers/ActionLogger/index.tsx`:
- Around line 49-51: The retrieved expandLevel from
api.getCurrentParameter<ActionsParameters['actions']>(PARAM_KEY) must be
normalized before calling this.setState; validate that the value is a finite
number, coerce to an integer (e.g. Math.floor), clamp to a safe range (minimum 0
or 1 as your UI expects), and fall back to the default (1) for NaN/invalid
inputs, then pass that sanitized value to this.setState({ expandLevel }); apply
the same normalization logic to the other assignment site around the 68-70 block
so both places use the same validated, clamped integer for expandLevel.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/addons/docs/src/blocks/controls/Color.stories.tsxcode/addons/docs/src/blocks/controls/Color.tsxcode/addons/docs/src/blocks/controls/types.tscode/core/src/actions/components/ActionLogger/index.tsxcode/core/src/actions/containers/ActionLogger/index.tsxcode/core/src/actions/types.ts
| const expandLevel = | ||
| api.getCurrentParameter<ActionsParameters['actions']>(PARAM_KEY)?.expandLevel ?? 1; | ||
| this.setState({ expandLevel }); |
There was a problem hiding this comment.
Normalize expandLevel before storing it in state.
expandLevel is user-configurable, so invalid numeric values (e.g. negative, fractional, NaN) can leak into state and produce inconsistent expansion behavior.
Suggested fix
+ private getExpandLevel = () => {
+ const raw = this.props.api.getCurrentParameter<ActionsParameters['actions']>(PARAM_KEY)?.expandLevel;
+ return Number.isInteger(raw) && raw >= 0 ? raw : 1;
+ };
+
override componentDidMount() {
@@
- const expandLevel =
- api.getCurrentParameter<ActionsParameters['actions']>(PARAM_KEY)?.expandLevel ?? 1;
- this.setState({ expandLevel });
+ this.setState({ expandLevel: this.getExpandLevel() });
}
@@
- const expandLevel =
- api.getCurrentParameter<ActionsParameters['actions']>(PARAM_KEY)?.expandLevel ?? 1;
- this.setState({ expandLevel });
+ this.setState({ expandLevel: this.getExpandLevel() });
};Also applies to: 68-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/actions/containers/ActionLogger/index.tsx` around lines 49 -
51, The retrieved expandLevel from
api.getCurrentParameter<ActionsParameters['actions']>(PARAM_KEY) must be
normalized before calling this.setState; validate that the value is a finite
number, coerce to an integer (e.g. Math.floor), clamp to a safe range (minimum 0
or 1 as your UI expects), and fall back to the default (1) for NaN/invalid
inputs, then pass that sanitized value to this.setState({ expandLevel }); apply
the same normalization logic to the other assignment site around the 68-70 block
so both places use the same validated, clamped integer for expandLevel.
Sidnioulz
changed the title
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks so much for this PR @mixelburg! I'll be very happy to approve it once we've cleared the mixup with the ActionLogger. See also improvements you can make to the stories!
Code LGTM besides that.
The ColorControl preset limit was previously hardcoded to 27 colors. This prevented users with large design systems (more than 27 colors) from displaying all their preset colors. Add a new optional maxPresetColors prop to ColorConfig/ColorControl that allows users to configure or remove this limit: - Defaults to 27 for backward compatibility - Setting to 0 or Infinity removes the limit entirely - Any other positive number limits presets to that count Add two new stories demonstrating the feature: - WithMaxPresetColors (limit to 5) - WithUnlimitedPresetColors (maxPresetColors: 0, 40 colors) Fixes storybookjs#20298
mixelburg
force-pushed
the
feat/color-preset-limit
branch
from
04577eb to
7383be3
Compare
|
Hi @Sidnioulz — the ActionLogger mixup is now resolved. The branch was forked before #33977 merged, so it included those commits in the diff. I've rebased onto the latest |
Thank you! We have two more feedback items to go through and then we can merge this:
Thanks |
…tartOpen to stories - Normalize maxPresetColors: fall back to default 27 for negative, non-integer, or NaN values - Add startOpen: true to WithMaxPresetColors and WithUnlimitedPresetColors stories for Chromatic captures
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.45 MB | 20.21 MB | 🎉 -245 KB 🎉 |
| Dependency size | 16.54 MB | 16.54 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 779 KB | 779 KB | 🎉 -116 B 🎉 |
| Dependency size | 67.65 MB | 67.40 MB | 🎉 -245 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 66.17 MB | 65.93 MB | 🎉 -245 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1.04 MB | 1.04 MB | 🎉 -385 B 🎉 |
| Dependency size | 37.00 MB | 36.75 MB | 🎉 -245 KB 🎉 |
| Bundle Size Analyzer | node | node |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/frameworks/svelte-vite/package.json`:
- Line 70: The change broadened the peerDependency for
"@sveltejs/vite-plugin-svelte" in package.json (the dependency string on that
package) which is outside this PR's stated scope; update the PR by either adding
a clear note in the PR description documenting this peerDependency change and
the "accidental downgrade" fix, or remove the change from this branch and put
the updated "@sveltejs/vite-plugin-svelte" peer range into a separate focused PR
so only the three ColorControl files remain in this PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44b48a5d-60c3-456b-9e16-e567c4ae4bf6
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
code/frameworks/svelte-vite/package.json
| }, | ||
| "peerDependencies": { | ||
| "@sveltejs/vite-plugin-svelte": "^2.0.0 || ^3.0.0 || ^4.0.0 || ^5.0.0 || ^6.0.0", | ||
| "@sveltejs/vite-plugin-svelte": "^2.0.0 || ^3.0.0 || ^4.0.0 || ^5.0.0 || ^6.0.0 || ^7.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if `@sveltejs/vite-plugin-svelte` version 7.x exists
curl -s "https://registry.npmjs.org/@sveltejs/vite-plugin-svelte" | jq '.versions | keys | map(select(startswith("7."))) | sort | .[-5:]'Repository: storybookjs/storybook
Length of output: 114
Document or split the peerDependency change.
This package.json change is outside the stated PR scope. According to the comments summary, the PR should contain "only the three ColorControl files," but this extends @sveltejs/vite-plugin-svelte peer compatibility to v7. While the version exists and the change is valid, either document it in the PR description (explaining the "accidental downgrade" fix mentioned in the commit) or split it into a separate PR for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/frameworks/svelte-vite/package.json` at line 70, The change broadened
the peerDependency for "@sveltejs/vite-plugin-svelte" in package.json (the
dependency string on that package) which is outside this PR's stated scope;
update the PR by either adding a clear note in the PR description documenting
this peerDependency change and the "accidental downgrade" fix, or remove the
change from this branch and put the updated "@sveltejs/vite-plugin-svelte" peer
range into a separate focused PR so only the three ColorControl files remain in
this PR.
Demonstrates fallback to default (27) when negative value is passed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Added a |
2 participants
What does this PR do?
Closes #20298
Adds a new optional
maxPresetColorsprop toColorConfig(and by extensionColorControl) that allows users to configure or remove the hardcoded 27-color preset limit.Problem
The
ColorControlcomponent has a hardcoded limit of 27 preset colors:This prevents users with design systems that have more than 27 colors from displaying all their presets in the color picker control.
Solution
Add a
maxPresetColors?: numberoption toColorConfig:27for backward compatibility0orInfinityremoves the limit entirelyUsage
Changes
types.ts: AddedmaxPresetColors?: numbertoColorConfiginterface with JSDocColor.tsx: UpdatedusePresetshook to accept and usemaxPresetColorsparameter; updatedColorControlto pass it throughColor.stories.tsx: Added two new stories demonstrating the feature (WithMaxPresetColorsandWithUnlimitedPresetColors)Summary by CodeRabbit
New Features
Examples
Documentation