UI: Add padding for ArgsTable shadow in TabbedArgsTable#33034
Conversation
Sidnioulz
added
ui
maintenance
|
View your CI Pipeline Execution ↗ for commit 9ff887c
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughTableWrapper in ArgsTable now computes a CSS Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
✨ 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: 0
🧹 Nitpick comments (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx (1)
109-109: Effective fix for the cropped shadow issue.The 3px horizontal padding correctly accommodates the drop-shadow filter's 3px blur radius (lines 121-122), allowing the shadow to render properly despite the TabPanel's
overflow: hiddenconstraint. Consider adding a brief inline comment explaining that the value matches the shadow blur radius, which would help future maintainers understand the relationship.- paddingInline: inTabPanel ? 3 : 0, + paddingInline: inTabPanel ? 3 : 0, // Matches drop-shadow blur radius to prevent cropping
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-09-19T15:49:02.264Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32270
File: code/core/src/components/components/AriaTabs/AriaTabPanel.tsx:27-30
Timestamp: 2025-09-19T15:49:02.264Z
Learning: In AriaTabPanel component, when hasScrollbar=false, the Panel container intentionally maintains overflowY: hidden. This forces consumers to implement their own scrolling within tab content rather than relying on automatic browser scrolling, preventing double scrollbars and giving full control over scroll behavior.
Applied to files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx (1)
20-25: Inconsistent AI summary.The AI-generated summary claims the
TableWrapperprops type now includespaddingInline?: number, but the props type remains unchanged. ThepaddingInlineadded on line 109 is a computed CSS property, not a component prop.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Sidnioulz
force-pushed
the
a11y/tabbed-argstable-cropped-shadow
branch
from
94fcff5 to
9ff887c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx (1)
109-109: Consider documenting the magic number.The value
3(theme units) is used without explanation. Consider adding a brief comment explaining why this specific value was chosen for the shadow padding.- paddingInline: inTabPanel ? 3 : 0, + // Padding to prevent shadow cropping in tab panels (3 theme units ≈ 24px) + paddingInline: inTabPanel ? 3 : 0,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-09-19T15:49:02.264Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32270
File: code/core/src/components/components/AriaTabs/AriaTabPanel.tsx:27-30
Timestamp: 2025-09-19T15:49:02.264Z
Learning: In AriaTabPanel component, when hasScrollbar=false, the Panel container intentionally maintains overflowY: hidden. This forces consumers to implement their own scrolling within tab content rather than relying on automatic browser scrolling, preventing double scrollbars and giving full control over scroll behavior.
Applied to files:
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx (1)
109-109: No changes required; the code fix is sound and follows established patterns.The
paddingInline: 3addition correctly provides horizontal breathing room for thedrop-shadowfilter on tbody (0px horizontal offset, 3px blur radius). This parallels the existing conditionalmarginInlinelogic on line 108 and applies only wheninTabPanelis true. The theme unit value (3 ≈ 24px) provides ample clearance for the shadow to render fully without clipping.The TabPanel's
overflowY: hiddenonly constrains vertical overflow and does not affect the horizontal shadow rendering, making the horizontal padding the appropriate fix for this context.
3 participants
What I did
See https://www.chromatic.com/test?appId=635781f3500dd2c49e189caf&id=6913769e99ba81e2674e568e
This one is opinionated. I cannot realistically remove the
overflow: hiddenthat causes the shadow cropping, as TabPanel now has support for a ScrollArea, implying a verticaloverflow-y: scroll, and so thatoverflow-x: visibleis impossible to achieve.Considering the impact here, and the benefits of natively supporting scrolling in TabPanels, I propose to just add enough padding for the shadow to render well. Alternatively, we could also just drop the shadow.
@MichaelArestad let me know what you think please!
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
http://localhost:6006/?path=/story/addons-docs-blocks-blocks-controls--subcomponents-of-story
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
✏️ Tip: You can customize this high-level summary in your review settings.