Core: Fix handling complex viewport sizes#33615
Conversation
|
View your CI Pipeline Execution ↗ for commit ebe0233
☁️ Nx Cloud last updated this comment at |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughInlines numeric/unit parsing and switches viewport/frame sizing to CSS calc(...); introduces per-axis locked state that disables width/height inputs and conditionally renders drag handles; updates numeric-input disabled styling; adds a "Calculated" custom viewport story; simplifies the viewport resize handler to always apply computed values. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NumericInput
participant Viewport
participant useViewport
participant StateStore
User->>NumericInput: edit numeric value or drag handle
NumericInput->>Viewport: emit new dimension value
Viewport->>useViewport: request apply/normalize (calc-based)
useViewport->>StateStore: update viewport state/globals
StateStore-->>Viewport: new dimensions & locked flags
Viewport-->>NumericInput: disable inputs / update displayed sizes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing Touches
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: 1
🤖 Fix all issues with AI agents
In `@code/core/src/manager/components/preview/Viewport.tsx`:
- Around line 275-292: The per-axis lock in the dimensions useMemo allows
resizing when one axis is complex; compute an isLocked flag (e.g., const
isLocked = !nx || !ny) inside the same useMemo and set locked: { width:
isLocked, height: isLocked } so both axes are locked if either dimension is
non-serializable; update any downstream checks that currently read locked.width
or locked.height to use the single isLocked value (or the updated locked object)
so resizing/resolution logic treats the pair as locked when either axis is
complex.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/NumericInput.tsx (1)
49-63: Consider a non-:has()fallback for disabled styling.
:has()support is still uneven in some environments. If Storybook manager must run in older browsers, the disabled styling won’t apply. A simple data-attribute fallback keeps visuals consistent.🔧 Possible fallback
const Wrapper = styled.div<{ after?: ReactNode; before?: ReactNode }>( ({ after, before, theme }) => ({ ... - '&:has(input:disabled)': { + '&:has(input:disabled), &[data-disabled="true"]': { background: theme.base === 'light' ? theme.color.lighter : theme.input.background, cursor: 'not-allowed', }, ... }) );- <Wrapper after={after} before={before} className={className} style={style}> + <Wrapper + after={after} + before={before} + className={className} + style={style} + data-disabled={props.disabled ? 'true' : undefined} + >
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
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/core/src/manager/components/preview/Viewport.stories.tsx`:
- Around line 133-142: The Calculated story currently disables Chromatic
snapshots and opts out of story tests, so the 'calc' viewport path isn't
covered; change the story configuration for Calculated (created via meta.story)
to keep it under automation by removing chromatic: { disableSnapshot: true } and
removing the tags: ['!test'] entry (or alternatively add a dedicated
unit/integration test that mounts the same Calculated scenario using the
customViewports and the viewport: { value: 'calc' } setting to exercise the
calc(...) code path). Ensure references to Calculated, meta.story,
customViewports and viewport: { value: 'calc' } are used so the same behavior is
covered by automated tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6400238-3c84-4646-a2e9-35120b32ddc3
📒 Files selected for processing (1)
code/core/src/manager/components/preview/Viewport.stories.tsx
|
I am not sure if these are considered blockers but there are a couple scenarios when interacting with the viewport inputs and trying to type "calc"
|
yannbf
left a comment
yannbf
left a comment
There was a problem hiding this comment.
LGTM but there are some minor issues, see my comment above
|
@yannbf |
2 participants
What I did
Fixed custom viewports with complex CSS values such as
calc(100% - 100px). These values now properly apply to the viewport, even when zooming, but cannot be updated since they aren't URL serializable.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Improvements
Testing