Skip to content

Commit e2cdb8e

Browse files
RoyEJohnsonclaudeCopilot
committed
Copilot suggestions
BodyPortal never uses style, so remove the style parameter from where ToastContainer calls it. Have Tabs merge style parameters Address Copilot code review comments 1. Fix Tabs style prop merge order - Changed from {...style, ...additionalStyle} to {...additionalStyle, ...style} - Allows consumers to override CSS variables via the style prop 2. Add style prop support to BodyPortal - Added style to BodyPortalProps type - Apply style to portal element in useLayoutEffect - Clean up styles on unmount - Pass style from BodyPortalToastContainer to BodyPortal - Fixes z-index CSS variable binding for toast containers 3. Remove unused icon prop from Tooltip - The icon prop is only used in TooltipGroup, not in Tooltip itself - Removed from Tooltip destructuring to avoid unused variable 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix BodyPortal to avoid teardown/reinsert on style changes Split the useLayoutEffect into two separate effects: 1. One for portal creation/destruction and non-style props (doesn't depend on style) 2. One for style updates only (depends on style but doesn't remove/reinsert the portal) This prevents unnecessary DOM churn when only CSS variables/styles change, which could cause focus loss or UI flicker in portal-based components like ToastContainer. Also added comprehensive tests for: - CSS variable application - Regular CSS property application - Style updates without portal removal - Style cleanup on unmount - Handling of null/undefined style values Addresses code review comments about performance and test coverage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address Copilot review comments for style handling and prop forwarding Fix three issues identified in code review: 1. BodyPortal: Convert camelCase to kebab-case for style cleanup - React style objects use camelCase (e.g., backgroundColor) - CSS removeProperty expects kebab-case (e.g., background-color) - Added conversion to prevent style property leaks on unmount 2. BodyPortalToastContainer: Memoize style object - Fresh style object on each render caused unnecessary portal remounts - Wrapped style object in useMemo to prevent BodyPortal useLayoutEffect from triggering on every toast list change 3. Tooltip: Prevent invalid props from reaching AriaTooltip - icon and ariaLabel are trigger-only props (used by TooltipGroup) - Now explicitly destructured to avoid spreading them to AriaTooltip - Prevents invalid DOM attributes and unexpected react-aria behavior All fixes maintain backward compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Handle CSS custom property copying Update ToastContainer.spec.tsx.snap Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 0393ff7 commit e2cdb8e

9 files changed

Lines changed: 139 additions & 9 deletions

File tree

src/components/BodyPortal.spec.tsx

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,4 +261,105 @@ describe('BodyPortal', () => {
261261
</body>
262262
`);
263263
});
264+
265+
it('applies CSS variables from style prop', () => {
266+
render(
267+
<BodyPortal
268+
slot='modal'
269+
style={{ '--my-variable': 'red', '--another-var': '10px' } as React.CSSProperties}
270+
>
271+
Modal content
272+
</BodyPortal>,
273+
{ container: root }
274+
);
275+
276+
const portal = document.body.querySelector('[data-portal-slot="modal"]') as HTMLElement;
277+
expect(portal).toBeTruthy();
278+
expect(portal.style.getPropertyValue('--my-variable')).toBe('red');
279+
expect(portal.style.getPropertyValue('--another-var')).toBe('10px');
280+
});
281+
282+
it('applies regular CSS properties from style prop', () => {
283+
render(
284+
<BodyPortal
285+
slot='modal'
286+
style={{ backgroundColor: 'blue', fontSize: '16px' }}
287+
>
288+
Modal content
289+
</BodyPortal>,
290+
{ container: root }
291+
);
292+
293+
const portal = document.body.querySelector('[data-portal-slot="modal"]') as HTMLElement;
294+
expect(portal).toBeTruthy();
295+
expect(portal.style.backgroundColor).toBe('blue');
296+
expect(portal.style.fontSize).toBe('16px');
297+
});
298+
299+
it('updates styles when style prop changes without removing portal element', () => {
300+
const TestComponent = ({ color }: { color: string }) => (
301+
<BodyPortal
302+
slot='modal'
303+
style={{ '--color': color } as React.CSSProperties}
304+
>
305+
Modal content
306+
</BodyPortal>
307+
);
308+
309+
const { rerender } = render(<TestComponent color='red' />, { container: root });
310+
311+
const portal = document.body.querySelector('[data-portal-slot="modal"]') as HTMLElement;
312+
expect(portal).toBeTruthy();
313+
expect(portal.style.getPropertyValue('--color')).toBe('red');
314+
315+
// Store reference to verify it's the same element after update
316+
const portalRef = portal;
317+
318+
rerender(<TestComponent color='blue' />);
319+
320+
const updatedPortal = document.body.querySelector('[data-portal-slot="modal"]') as HTMLElement;
321+
expect(updatedPortal).toBe(portalRef); // Should be the same element
322+
expect(updatedPortal.style.getPropertyValue('--color')).toBe('blue');
323+
});
324+
325+
it('removes styles on unmount', () => {
326+
const { unmount } = render(
327+
<BodyPortal
328+
slot='modal'
329+
style={{ '--my-variable': 'red', backgroundColor: 'blue' } as React.CSSProperties}
330+
>
331+
Modal content
332+
</BodyPortal>,
333+
{ container: root }
334+
);
335+
336+
const portal = document.body.querySelector('[data-portal-slot="modal"]') as HTMLElement;
337+
expect(portal).toBeTruthy();
338+
expect(portal.style.getPropertyValue('--my-variable')).toBe('red');
339+
expect(portal.style.backgroundColor).toBe('blue');
340+
341+
unmount();
342+
343+
// Portal element should be removed from DOM
344+
expect(document.body.querySelector('[data-portal-slot="modal"]')).toBeNull();
345+
});
346+
347+
it('handles null and undefined style values gracefully', () => {
348+
render(
349+
<BodyPortal
350+
slot='modal'
351+
style={{ '--defined': 'red', '--null': null, '--undefined': undefined } as React.CSSProperties}
352+
>
353+
Modal content
354+
</BodyPortal>,
355+
{ container: root }
356+
);
357+
358+
const portal = document.body.querySelector('[data-portal-slot="modal"]') as HTMLElement;
359+
expect(portal).toBeTruthy();
360+
expect(portal.style.getPropertyValue('--defined')).toBe('red');
361+
// null and undefined values should not be set
362+
expect(portal.style.getPropertyValue('--null')).toBe('');
363+
expect(portal.style.getPropertyValue('--undefined')).toBe('');
364+
});
264365
});

src/components/BodyPortal.tsx

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export type BodyPortalProps = React.PropsWithChildren<{
3333
}>;
3434

3535
export const BodyPortal = React.forwardRef<HTMLElement, BodyPortalProps>((
36-
{ children, className, role, slot, tagName, id, ariaLabel, ...props }, ref?: React.ForwardedRef<HTMLElement>
36+
{ children, className, role, slot, tagName, id, ariaLabel, style, ...props }, ref?: React.ForwardedRef<HTMLElement>
3737
) => {
3838
const tag = tagName?.toUpperCase() ?? 'DIV';
3939
const internalRef = React.useRef<HTMLElement | null>(
@@ -53,6 +53,7 @@ export const BodyPortal = React.forwardRef<HTMLElement, BodyPortalProps>((
5353
const bodyPortalOrderedRefs = React.useContext(BodyPortalSlotsContext);
5454
const testId = props['data-testid'];
5555

56+
// Effect for creating/destroying the portal element and setting non-style props
5657
React.useLayoutEffect(() => {
5758
const element = internalRef.current;
5859
if (!element) { return; }
@@ -90,6 +91,28 @@ export const BodyPortal = React.forwardRef<HTMLElement, BodyPortalProps>((
9091
};
9192
}, [bodyPortalOrderedRefs, className, id, role, slot, ariaLabel, tag, testId]);
9293

94+
// Separate effect for updating styles without removing/reinserting the portal
95+
React.useLayoutEffect(() => {
96+
const element = internalRef.current;
97+
if (!element || !style) { return; }
98+
99+
// Apply styles
100+
Object.entries(style).forEach(([key, value]) => {
101+
if (value == null) return;
102+
const cssKey = key.startsWith('--') ? key : key.replace(/([A-Z])/g, '-$1').toLowerCase();
103+
element.style.setProperty(cssKey, String(value));
104+
});
105+
106+
// Cleanup: remove styles on unmount or when style prop changes
107+
return () => {
108+
Object.keys(style).forEach(key => {
109+
// Convert camelCase to kebab-case for CSS property names
110+
const cssKey = key.startsWith('--') ? key : key.replace(/([A-Z])/g, '-$1').toLowerCase();
111+
element.style.removeProperty(cssKey);
112+
});
113+
};
114+
}, [style]);
115+
93116
if (!internalRef.current) { return null; }
94117

95118
return createPortal(children, internalRef.current);

src/components/Tabs.css

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
}
3434

3535
/* Default tabs variant (horizontal with underline) */
36-
.tabs[data-orientation="horizontal"] [role="tablist"] {
36+
.tabs:not(.tabs-button-bar)[data-orientation="horizontal"] [role="tablist"] {
3737
border-bottom: 0.1rem solid var(--tabs-border-color, #d5d5d5);
3838
}
3939

src/components/Tabs.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,15 @@ export type TabsProps = {
99
size?: "large" | "medium" | "small";
1010
className?: string;
1111
children?: React.ReactNode;
12+
style?: React.CSSProperties;
1213
} & RAC.TabsProps;
1314

1415
export const Tabs = ({
1516
variant,
1617
size = "medium",
1718
className,
1819
children,
20+
style,
1921
...restProps
2022
}: TabsProps) => {
2123
const tabsClass = classNames('tabs', {
@@ -25,7 +27,7 @@ export const Tabs = ({
2527
'tabs-large': size === 'large',
2628
}, className);
2729

28-
const style = {
30+
const additionalStyle = {
2931
'--tabs-border-color': palette.pale,
3032
'--tabs-active-border-color': palette.darkGreen,
3133
'--tabs-button-selected-bg': palette.neutralLight,
@@ -35,7 +37,7 @@ export const Tabs = ({
3537
return (
3638
<RAC.Tabs
3739
className={tabsClass}
38-
style={style}
40+
style={{...additionalStyle, ...style}}
3941
{...restProps}
4042
>
4143
{children}

src/components/Toast.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ export const Toast = ({
6363
});
6464

6565
const style = {
66-
'--toast-success-title-color': palette.darkerGreen,
66+
'--toast-success-title-color': palette.darkerGreen.startsWith('#') ? palette.darkerGreen : `#${palette.darkerGreen}`,
6767
'--toast-success-bg': palette.paleGreen,
6868
'--toast-neutral-title-color': palette.neutralDarker,
6969
'--toast-neutral-bg': palette.neutralLighter,

src/components/ToastContainer.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import React from 'react';
12
import { BodyPortal } from './BodyPortal';
23
import { Toast } from './Toast';
34
import { zIndex } from '../theme';
@@ -46,9 +47,9 @@ export const BodyPortalToastContainer: ToastContainerComponent = ({ toasts, onDi
4647
'toast-container-inline': inline,
4748
}, className);
4849

49-
const style = {
50+
const style = React.useMemo(() => ({
5051
'--toast-container-z-index': zIndex.toasts,
51-
} as React.CSSProperties;
52+
} as React.CSSProperties), []);
5253

5354
return (
5455
<BodyPortal className={containerClass} aria-live='polite' slot='toast' style={style}>

src/components/ToggleButtonGroup/styles.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { ToggleButtonGroup, ToggleButton } from "react-aria-components";
33
import { colors } from "../../theme";
44

55
export const StyledToggleButtonGroup = styled(ToggleButtonGroup)`
6-
// formerly {tabListBaseCss}
6+
/* formerly tabListBaseCss */
77
overflow-x: auto;
88
overscroll-behavior: contain;
99
display: flex;

src/components/Tooltip.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import React from 'react';
12
import { Button, OverlayArrow, Tooltip as AriaTooltip, TooltipTrigger } from 'react-aria-components';
23
import { Info } from './svgs/Info';
34
import {mergeProps, Placement, useTooltip} from 'react-aria';
@@ -11,7 +12,7 @@ type TooltipProps = {
1112
ariaLabel?: string;
1213
};
1314

14-
export const Tooltip = ({children, placement, icon, ...props}: React.PropsWithChildren<TooltipProps>) => {
15+
export const Tooltip = ({children, placement, icon, ariaLabel, ...props}: React.PropsWithChildren<TooltipProps>) => {
1516
const style = {
1617
'--tooltip-bg': palette.white,
1718
'--tooltip-color': palette.neutralThin,

src/components/__snapshots__/ToastContainer.spec.tsx.snap

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ exports[`BodyPortalToastContainer matches snapshot 1`] = `
88
<div
99
class="toast-container"
1010
data-portal-slot="toast"
11+
style="--toast-container-z-index: 40;"
1112
>
1213
<div
1314
class="toast"
@@ -78,6 +79,7 @@ exports[`BodyPortalToastContainer uses inline prop 1`] = `
7879
<div
7980
class="toast-container toast-container-inline"
8081
data-portal-slot="toast"
82+
style="--toast-container-z-index: 40;"
8183
>
8284
<div
8385
class="toast toast-inline"

0 commit comments

Comments
 (0)