Skip to content

Conversation

Copy link

Copilot AI commented Sep 23, 2025

Fixes React DOM warnings caused by styled-components passing styling-only props to underlying DOM elements. Updated components to use the $ prefix convention for transient props that should not be passed through to the DOM.

Problem

Styled-components was passing all props to underlying DOM elements, causing React to throw warnings for props that don't exist on DOM elements. This occurred because styled-components by default forwards all props to the wrapped component unless they are prefixed with $.

Example warning:

Warning: React does not recognize the `vertical` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `vertical` or explicitly set it using setAttribute.

Solution

Updated styled-components to use the $ prefix convention for props that are only used for styling logic and should not be passed to DOM elements. This follows the styled-components v5.1+ recommendation.

Before:

const StyledTabListItem = styled.div<IReqoreTabListItemStyle>`
  ${({ disabled, vertical, fill, fixed, padded }) => {
    // styling logic using these props
  }}
`;

// Usage - all props get passed to DOM
<StyledTabListItem
  disabled={disabled}
  vertical={vertical}
  fill={fill}
  // These props end up on the DOM element ❌
/>

After:

const StyledTabListItem = styled.div<IReqoreTabListItemStyle>`
  ${({ $disabled, $vertical, $fill, $fixed, $padded }) => {
    // styling logic using prefixed props
  }}
`;

// Usage - $ prefixed props are filtered out
<StyledTabListItem
  $disabled={disabled}
  $vertical={vertical}
  $fill={fill}
  // These props are NOT passed to DOM ✅
/>

Components Fixed

COMPLETED FILES (24 components with multiple styled components):

  • Tabs components (item.tsx, list.tsx, index.tsx, content.tsx) - Fixed styling props like disabled, vertical, fill, padded, size
  • Menu component - Fixed props like position, transparent, rounded, isResizable*
  • Input component - Fixed props like fluid, minimal, readOnly, disabled, pill
  • Checkbox component - Fixed props across multiple styled components including switches and text wrappers
  • Spacer component - Fixed props like horizontal, vertical, width, height
  • Textarea component - Fixed wrapper and input styling props (fluid, fixed, minimal, etc.)
  • Layout component - Fixed sidebar positioning prop (withSidebar)
  • Breadcrumbs component - Fixed size, padding, margin, responsive props
  • Collection component - Fixed content overflow and selection state props
  • Comment & CommentFeed components - Fixed margin and gap styling props
  • Navbar components (4 files: divider, group, item, main) - Fixed positioning, state, and layout props
  • Columns component - Fixed grid layout styling props
  • Notifications components (2 files) - Fixed complex styling props including type, intent, size, positioning
  • Tree component - Fixed size and expandable styling props
  • Tag group component - Fixed layout and alignment styling props (wrap, fluid, gapSize, align)
  • Table main component - Fixed pinning and layout props (isPinned, rounded, size)

Impact

  • ✅ Eliminates React DOM warnings for styling-only props in 24 components
  • ✅ Maintains all existing functionality (all tests passing)
  • ✅ 60% progress on fixing the component library
  • ✅ Establishes clear pattern for fixing remaining components (~16 remaining)
  • ✅ No breaking changes to component APIs

Pattern for Remaining Work

This PR establishes the pattern for fixing the remaining components in the codebase:

  1. Identify props used only for styling logic in styled component templates
  2. Add $ prefix to those props in TypeScript interfaces
  3. Update styled component templates to use $ prefixed props
  4. Update component usage to pass $ prefixed props
  5. Leave DOM attributes (width, height, className) and styled-components special props (theme, as) without prefix

The foundation is now in place for completing this work across the entire component library.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] This repo is using styled components. Whenever I pass a prop to SC (one that I just want to use in the styled-component for some logic etc), it tries to drill that prop all the way to the underlying DOM element. This causes react to panic because the p... Fix styled-components props to prevent React DOM warnings Sep 23, 2025
Copilot AI requested a review from Foxhoundn September 23, 2025 09:00
Copilot finished work on behalf of Foxhoundn September 23, 2025 09:00
theme={theme}
checked={checked}
$checked={checked}
parentHasGradient={!!effect?.gradient}
Copy link
Member

@Foxhoundn Foxhoundn Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot It's missing here for labelPosition, effect and parentHasGradient

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit af9491f. Added $ prefix for parentHasGradient prop and removed unused labelPosition prop from StyledSwitch component. The effect prop is kept as it's used by the styled-components system for the interactive styling.

@Foxhoundn
Copy link
Member

@copilot can you continue your work?

Copy link
Author

Copilot AI commented Sep 23, 2025

@copilot can you continue your work?

Yes, I've addressed the feedback about the missing $ prefix for parentHasGradient and removed unused props from the StyledSwitch component. All tests are passing (226/226). The checkbox component now correctly prevents React DOM warnings for styling-only props.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • o30291.ingest.sentry.io
    • Triggering command: /usr/local/bin/node wrapper_dist/bootstrap.js exec (dns block)
  • static.snyk.io
    • Triggering command: /usr/local/bin/node wrapper_dist/bootstrap.js exec (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from Foxhoundn September 23, 2025 09:34
Copilot finished work on behalf of Foxhoundn September 23, 2025 09:34
@Foxhoundn
Copy link
Member

@copilot Can you do more components? There is still 30+ left.

Copy link
Author

Copilot AI commented Sep 23, 2025

@copilot Can you do more components? There is still 30+ left.

Absolutely! I've continued the work and fixed 4 more components in commit 4367736:

  • Textarea - Fixed wrapper and input styling props (fluid, fixed, minimal, etc.)
  • Layout - Fixed sidebar positioning prop (withSidebar)
  • Breadcrumbs - Fixed size, padding, margin, responsive props
  • Collection - Fixed content overflow and selection state props

That brings us to 11 components completed out of ~40 total. I'm working systematically through the component library, applying the same $ prefix pattern to prevent React DOM warnings. Should I continue with more components?

Copilot finished work on behalf of Foxhoundn September 23, 2025 10:14
@Foxhoundn
Copy link
Member

@copilot yes please, continue with 10 more

Copy link
Author

Copilot AI commented Sep 23, 2025

@copilot yes please, continue with 10 more

Done! I've completed 10 more components as requested in commits fcf0468 and aea289e:

Batch 1 (6 components):

  • Comment & CommentFeed - Fixed margin and gap styling props
  • All 4 Navbar components (divider, group, item, main) - Fixed positioning, state, and layout props
  • Columns - Fixed grid layout styling props

Batch 2 (4 components):

  • Notifications (both files) - Complex component with many styling props (type, intent, size, margin, etc.)
  • Tree - Fixed size and expandable styling props

That brings us to 21 components completed out of ~40 total. The remaining major components include Button, Panel, Tag, and Table components. Should I continue with the next batch?

Copilot finished work on behalf of Foxhoundn September 23, 2025 10:59
@Foxhoundn
Copy link
Member

@copilot yes, please. continue

Copy link
Author

Copilot AI commented Sep 23, 2025

@copilot yes, please. continue

Continuing! I've made good progress and completed 3 more components in commits 9423102 and 9ede684:

Latest batch:

  • Tabs content - Fixed padding and size styling props
  • Tag group - Fixed layout and alignment styling props (wrap, fluid, gapSize, align)
  • Table main component - Fixed pinning and layout props (isPinned, rounded, size)

That brings us to 24 components completed (60% progress). The remaining major components are Button, Panel, Tag main component, and Table sub-components (cell, header, row). These are more complex with many styling props, but I'm working through them systematically.

Copilot finished work on behalf of Foxhoundn September 23, 2025 11:32
@Foxhoundn
Copy link
Member

@copilot please do more components

Foxhoundn added 4 commits September 24, 2025 15:51
…t, InternalPopover, Message, and Notifications components
…, Columns, ControlGroup, Effect, InternalPopover, Notifications, and Table components
…eader to improve prop handling and prevent unwanted prop forwarding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants