UI: Fix Copy button overlapping code in portrait mode #33420
Conversation
Fixes storybookjs#23642 Increased paddingRight in syntaxhighlighter.tsx to 85px. This prevents the code text from running under the absolute positioned "Copy" button when viewing in mobile/portrait modes.
📝 WalkthroughWalkthroughSyntaxHighlighter UI layout adjusted: wrapper now uses flex with wrapping and gap, scroller internals set to fit-content, code block right padding removed, a RelativeActionBar variant introduced for copyable code, and minor line-number/comment cleanup. ToggleButton story layout set to fullscreen. No public API changes. Changes
Sequence Diagram(s)(omitted — changes are styling/storybook tweaks and do not introduce multi-component control flow requiring sequence diagrams) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (1)
120-126: The hardcoded85pxcorrectly prevents the Copy button overlap—extracting to a constant is optional.The fix successfully addresses the portrait-mode overlap issue. The ActionBar is absolutely positioned at the bottom-right of the Wrapper, and the
85pxpadding on the Code component provides sufficient clearance for the Copy button (which requires ~65-70px with padding and text). This is a proportionate solution to the layout issue.However, extracting the magic number to a named constant would improve readability:
const CODE_RIGHT_PADDING_FOR_ACTION_BAR = 85; // Clearance for absolutely-positioned Copy buttonNote: The file already contains other hardcoded values (e.g.,
paddingLeft: 2with a TODO), so extraction here is a code quality improvement rather than a necessary refactoring.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
🧠 Learnings (1)
📓 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.
⏰ 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). (1)
- GitHub Check: normal
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (1)
116-117: Consider a responsive approach to avoid excessive padding on desktop.The fixed 85px clearance applies uniformly across all viewport sizes. This replaces
theme.layoutMargin(10px), which was theme-aware. While this value provides adequate clearance for the Copy button (likely 40-80px depending on content), it adds substantial padding on desktop where the overlap issue doesn't occur.Consider:
- Responsive solution: Use media queries to apply this clearance only in portrait/mobile viewports, preserving the original
theme.layoutMarginfor desktop.- Derive from ActionBar: Define this constant in a shared location or export it from ActionBar to avoid hidden coupling.
This approach works for the immediate bug fix but could improve desktop presentation and maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.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-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (1)
code/core/src/components/index.ts (1)
Code(9-9)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (1)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (1)
126-126: Fix correctly prevents overlap; architectural concerns noted separately.The change from
theme.layoutMargintoACTION_BAR_CLEARANCEsuccessfully creates the necessary space to prevent code text from running under the absolutely-positioned Copy button in portrait mode, addressing issue #23642.The implementation is correct for the immediate fix. Broader concerns about responsive design and hard-coded dimensions are addressed in the comment on lines 116-117.
There was a problem hiding this comment.
Thanks for your PR @Maelryn!
I don't think the proposed fix is going in the right direction, because:
- A large padding removes useful space from the
prearea, making it harder to read code snippets without scrolling, especially in portrait mode - The value is arbitrary and will not account for future changes in copy or for font size zoom settings
Instead of reserving space horizontally, I'd like the ActionBar div to reserve space vertically whenever there isn't enough room for both the ActionBar and the content.
This can be done with a flex layout on the parent of the pre and ActionBar, flex wrapping, and fit-content width across all divs/pres in the pre block with the ScrollArea.
I've put together a diff to get you started:
diff --git a/code/core/src/components/components/ToggleButton/ToggleButton.stories.tsx b/code/core/src/components/components/ToggleButton/ToggleButton.stories.tsx
index c1245b23274..38d821df485 100644
--- a/code/core/src/components/components/ToggleButton/ToggleButton.stories.tsx
+++ b/code/core/src/components/components/ToggleButton/ToggleButton.stories.tsx
@@ -10,6 +10,7 @@ const meta = {
title: 'ToggleButton',
component: ToggleButton,
tags: ['autodocs'],
+ parameters: { layout: 'fullscreen' },
args: { ariaLabel: false, children: 'Click me' },
} satisfies Meta<typeof ToggleButton>;
diff --git a/code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx b/code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
index 58286cec773..2451eba1b3b 100644
--- a/code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
+++ b/code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
@@ -69,6 +69,9 @@ const Wrapper = styled.div<WrapperProps>(
({ theme }) => ({
position: 'relative',
overflow: 'hidden',
+ display: 'flex',
+ display: 'flex',
+ flexWrap: 'wrap',
+ gap: theme.layoutMargin,
color: theme.color.defaultText,
}),
({ theme, bordered }) =>
@@ -98,6 +101,13 @@ const UnstyledScroller = ({ children, className }: ScrollAreaProps) => (
const Scroller = styled(UnstyledScroller)(
{
position: 'relative',
+ width: 'fit-content',
+ '> div': {
+ width: 'fit-content',
+ '> div > pre': {
+ width: 'fit-content',
+ },
+ },
},
({ theme }) => themedSyntax(theme)
);
@@ -125,6 +135,12 @@ const Code = styled.div(({ theme }) => ({
fontFamily: theme.typography.fonts.mono,
}));
+const RelativeActionBar = styled(ActionBar)({
+ position: 'relative',
+ marginLeft: 'auto',
+ alignSelf: 'flex-end',
+});
+
const processLineNumber = (row: any) => {
const children = [...row.children];
const lineNumberNode = children[0];
@@ -247,7 +263,7 @@ export const SyntaxHighlighter = ({
</Scroller>
{copyable ? (
- <ActionBar actionItems={[{ title: copied ? 'Copied' : 'Copy', onClick }]} />
+ <RelativeActionBar actionItems={[{ title: copied ? 'Copied' : 'Copy', onClick }]} />
) : null}
</Wrapper>Note that I forced one story to use a fullscreen layout as it helps spot the bug. The same logic would need to be applied to other code paths that use ActionBar, e.g.:
code/addons/docs/src/blocks/components/Preview.tsx(for this issue, that should be the "Code" button)addons/vitest/src/components/GlobalErrorModal.tsx(bonus)core/src/actions/components/ActionLogger/index.tsx(bonus)
Ideally, I'd like ActionBar to not have absolute positioning (cc @MichaelArestad in case you have planned a long-term replacement for that UI pattern). But if we want to change the positioning from absolute to relative, we'll also need to
- Wait for Storybook 11 to release the PR
- Add a note in the MIGRATION.md file
- Update addons that use ActionBar, such as the Jest addon
|
View your CI Pipeline Execution ↗ for commit b468bc9 ☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (1)
103-109: Consider making nested selectors more maintainable.The deep nested selectors (
> div > div > pre) couple this component to the internal DOM structure ofScrollAreaandReactSyntaxHighlighter. If these dependencies change their internal structure in future versions, these selectors will break silently.Consider:
- Adding data attributes or class names to target elements more reliably
- Documenting why these specific selectors are needed
- Adding a comment warning about the dependency on internal structure
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/components/components/ToggleButton/ToggleButton.stories.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/components/components/ToggleButton/ToggleButton.stories.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/components/components/ToggleButton/ToggleButton.stories.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/components/components/ToggleButton/ToggleButton.stories.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/ToggleButton/ToggleButton.stories.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/components/components/ToggleButton/ToggleButton.stories.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.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-11-05T09:36:55.944Z
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.
Applied to files:
code/core/src/components/components/ToggleButton/ToggleButton.stories.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (1)
code/core/src/components/index.ts (1)
ActionBar(43-43)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (6)
code/core/src/components/components/ToggleButton/ToggleButton.stories.tsx (1)
13-13: Clarify the relationship to the PR objective.This change adds a fullscreen layout parameter to the ToggleButton stories, but the PR objective is to fix the Copy button overlap in the SyntaxHighlighter component in portrait mode. Can you clarify why this change is included in this PR?
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (5)
125-130: LGTM - Code padding adjusted for flex layout.The removal of
paddingRightis appropriate since the flex layout withgapandRelativeActionBarnow handles the spacing between the code content and the Copy button. The minimalpaddingLeft: 2maintains proper text positioning.
132-136: Clean abstraction for the positioned copy button.The
RelativeActionBarcomponent provides a clear, maintainable solution for positioning the Copy button. The combination ofmarginLeft: 'auto'andalignSelf: 'flex-end'properly positions it at the bottom-right in the flex layout, and it will wrap appropriately on narrow viewports due to the parent'sflexWrap: 'wrap'.
250-252: LGTM - ActionBar replaced with RelativeActionBar.The swap from
ActionBartoRelativeActionBaris straightforward and maintains the same functional behavior while applying the new positioning logic to fix the overlap issue.
226-254: Verify the fix works across viewports and doesn't break scrolling.While the flexbox refactor is a solid approach, the PR description states no automated tests were added. Given this is a significant layout change, please verify:
- The Copy button no longer overlaps code in portrait/mobile viewports
- The layout remains correct in landscape/desktop viewports
- Horizontal scrolling still works properly for very long code lines
- The ActionBar wrapping behavior is acceptable when it moves to a new line
- Code selection/copying isn't interfered with by the ActionBar positioning
Consider adding visual regression tests or documenting the manual testing performed in detail.
72-74: The flex layout with wrapping is the correct approach.The flex layout using
display: 'flex',flexWrap: 'wrap', andgap: theme.layoutMarginis appropriate and doesn't break existing usages. The wrapping behavior allows the ActionBar to respond to narrow viewports by moving to its own line when needed, which is the desired responsive pattern. The RelativeActionBar'smarginLeft: 'auto'andalignSelf: 'flex-end'positioning work correctly within this flex context. Component usages across multiple addons (Onboarding, Docs, A11y) and the documented stories show no issues.
|
@Sidnioulz Thanks for the snippet! |
4 participants
Closes #23642
What I did
I increased the
paddingRightin theSyntaxHighlightercomponent to85px.This prevents the code text from running under the absolute positioned "Copy" button when viewing in mobile/portrait modes.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.