Refactor components to use polymorphic utility types #732

vineethasok · 2025-12-17T12:09:23Z

Replaces custom polymorphic type logic in Container, GridContainer, EllipsisContent, Link, and Text components with shared utility types from src/utils/polymorphic. Adds src/utils/polymorphic with documentation and type tests to ensure type safety and ref forwarding for polymorphic components. This centralizes and standardizes the polymorphic pattern across the codebase.

We can reuse this in all the places where we need to extend the content with component prop in the future prs

Replaces custom polymorphic type logic in Container, GridContainer, EllipsisContent, Link, and Text components with shared utility types from src/utils/polymorphic. Adds src/utils/polymorphic with documentation and type tests to ensure type safety and ref forwarding for polymorphic components. This centralizes and standardizes the polymorphic pattern across the codebase.

vercel · 2025-12-17T12:09:30Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
click-ui	Ready	Preview, Comment	Dec 17, 2025 0:11am

Copilot

Pull request overview

This PR introduces a centralized polymorphic component pattern by creating reusable utility types in src/utils/polymorphic. The refactoring eliminates duplicated type definitions across five components (Container, GridContainer, EllipsisContent, Link, and Text), replacing custom implementations with standardized PolymorphicComponent, PolymorphicComponentProps, PolymorphicProps, and PolymorphicRef types.

Key changes:

Added shared polymorphic utility types with comprehensive documentation and type tests
Refactored five components to use the centralized utilities, reducing code duplication
Maintained backward compatibility while improving type safety and consistency

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/utils/polymorphic/index.ts	Defines reusable polymorphic utility types for type-safe component composition
src/utils/polymorphic/index.test.tsx	Provides type safety tests demonstrating usage patterns with native and custom components
src/utils/polymorphic/README.md	Documents polymorphic component patterns, implementation guide, and usage examples
src/components/Typography/Text/Text.tsx	Refactored to use shared polymorphic utilities, removing duplicate type definitions
src/components/Link/Link.tsx	Refactored to use shared polymorphic utilities, removing duplicate type definitions
src/components/GridContainer/GridContainer.tsx	Refactored to use shared polymorphic utilities, removing duplicate type definitions
src/components/EllipsisContent/EllipsisContent.tsx	Refactored to use shared polymorphic utilities, removing duplicate type definitions
src/components/Container/Container.tsx	Refactored to use shared polymorphic utilities, removing duplicate type definitions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T12:09:48Z

src/utils/polymorphic/README.md

+```tsx
+<Container component="main" padding="lg" gap="xl" orientation="vertical">
+  <Container component="header" gap="md">
+    <Link component="h1" size="xl" weight="bold">


Using a Link component as a heading (h1) may be semantically incorrect and confusing for API consumers. Headings should typically use the Text component. Consider using <Text component=\"h1\"> in the example instead.

Copilot · 2025-12-17T12:09:49Z

src/utils/polymorphic/README.md

+  </Container>
+
+  <Container component="section" gap="md" orientation="vertical">
+    <Link component="h2" size="lg" weight="semibold">


Similar to the h1 example, using Link as h2 is semantically confusing. The example should use <Text component=\"h2\"> for heading elements to demonstrate proper semantic usage.

Copilot · 2025-12-17T12:09:49Z

src/utils/polymorphic/README.md

+      <Link component="li" size="md">Feature 1</Link>
+      <Link component="li" size="md">Feature 2</Link>
+      <Link component="li" size="md">Feature 3</Link>


Using Link components as list items (li) without href attributes or links is semantically incorrect. Consider using <Text component=\"li\"> for non-interactive list items in the example.

Copilot · 2025-12-17T12:09:49Z

src/utils/polymorphic/index.test.tsx

+          component="li"
+          size="md"
+        >
+          ✅ Works with styled-components


The comment says 'Works with styled-components' but the PR description mentions 'SCSS modules' and components use both styled-components and SCSS. This inconsistency should be clarified to accurately reflect the implementation.

ariser

the change request if for Polymorphic types, they can provide better developer experience

ariser · 2025-12-17T12:26:24Z

src/utils/polymorphic/index.test.tsx

+// ============================================================================
+// Test 3: Type Errors (These should NOT compile if uncommented)
+// ============================================================================
+
+// Example type errors that would be caught:
+// 1. div doesn't have 'disabled'
+// 2. span doesn't have 'href'
+// 3. Invalid Container prop values
+// 4. Invalid Link color values


What is this supposed to do?
We can add all these examples with // @ts-expect-error to verify that TS does not indeed accept them.
Typecheck will fail in case there is no type error.

ariser · 2025-12-17T12:33:34Z

src/utils/polymorphic/index.ts

+ * This uses a mapped type to properly infer the element type
+ */
+export type PolymorphicComponent<
+  TProps extends PolymorphicComponentProps<ElementType>,


TProps should't extend PolymorphicComponentProps

Right now you're required to explicitly extend your props from PolymorphicComponentProps in order to use this interface, it shouldn't be necessary

PolymorphicComponent should accept any interface, and simply inject polymorphic props into it

I should be able to do this:

interface MyComponentProps { text: string; } const MyComponent: PolymorphicComponent<MyComponentProps, 'p'> = (props) => { const { component, text } = props; };

or this

interface MyComponentProps { text: string; } const MyComponent = forwardRef< PolymorphicRef<'p'>, PolymorphicComponent<MyComponentProps, 'p'> >((props, ref) => { const { component, text } = props; });

ariser · 2025-12-17T12:54:12Z

src/utils/polymorphic/index.ts

+ */
+export type PolymorphicProps<
+  T extends ElementType,
+  TProps extends PolymorphicComponentProps<T>,


same here, it should not expect TProps to extend PolymorphicComponentProps, it should extend them itself

ariser · 2025-12-17T13:06:46Z

src/utils/polymorphic/index.ts

+}
+
+/**
+ * Merges the component's custom props with native HTML element props


thissss is a pain point for me

This is not something we need to change in this PR. Because this will be a breaking change, and also because it requires more thought put into it. But since we touched this area, I wanted to share my concerns.

I don't like that our components API include every single one of HTML attributes.

It makes it so much harder to find the actual prop you need using autocompletion. Half of the list is aria-attributes.

In some cases we end up with very similar properties:

content vs text vs description vs value... which one is the actual prop that component expects, and which ones are the default HTML ones?

every single onEvent prop; which one the component actually uses? Vs which ones are there just because HTML element accepts them

In some cases, it's not immediately obvious which element will receive these HTML attributes, if the component have multiple layers.

A better approach here, in my opinion, would be to have a separate property, smth like htmlAttributes, controlProps, we need to think of a good name. It will

still offer a way to pass any default attribute

avoid the conflict that we now have: if a custom prop name collides with HTML attr name, you'd be able to still pass both. You can't now.

provide a way to indicate, which HTML element will actually receive these props: via prop name, as well as via jsdoc if needed. E.g., controlProps on a TextField will clearly assign the props to the input element, rather than to the wrapping div.

allow to have multiple props to provide extra html attrs to multiple internal components, if needed. For example, separate html props for heading and for content area.

@ariser From what I understood, I think I can make the autocomplete to have priority than the html ones for now
But introducing a new prop to have the html attributes can lead to breaking changes for the smaller releases for now
I could included both the options of prop type htmlAtrributes or contrlProps and then remove it on our 0.1.0 and we can from then on maintain the new format

I'm not 100% sure if this is what you meant.
The main purpose for this component is reusability and implementing dry concept and maintaining the current logic to avoid breaking changes.

Yea, this is me sharing my general thoughts about a part of the code you touched. Let's not try to address this in this PR. We should discuss this in a wider group perhaps as well.

ariser · 2025-12-17T13:10:00Z

src/utils/polymorphic/README.md

+✅ **Full Type Safety** - TypeScript knows which props are valid based on the `component` prop
+✅ **DRY Implementation** - Reusable utility types in `@/utils/polymorphic`
+✅ **Ref Forwarding** - Properly typed refs for any element type
+✅ **Native & Custom Components** - Pass any HTML element or React component
+✅ **Works with Styled Components** - Compatible with styled-components and SCSS modules


it's not a list

vineethasok requested review from ariser, Copilot, fneves and hoorayimhelping December 17, 2025 12:09

vineethasok self-assigned this Dec 17, 2025

Copilot AI reviewed Dec 17, 2025

View reviewed changes

vercel bot deployed to Preview December 17, 2025 12:09 View deployment

Prettify content

8044ae3

vercel bot deployed to Preview December 17, 2025 12:11 View deployment

ariser requested changes Dec 17, 2025

View reviewed changes

Refactor components to use polymorphic utility types #732

Are you sure you want to change the base?

Refactor components to use polymorphic utility types #732

Uh oh!

Conversation

vineethasok commented Dec 17, 2025

Uh oh!

vercel bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

ariser left a comment

Choose a reason for hiding this comment

Uh oh!

ariser Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

ariser Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

ariser Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

ariser Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

ariser Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vineethasok Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

ariser Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

ariser Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel bot commented Dec 17, 2025 •

edited

Loading

ariser Dec 17, 2025 •

edited

Loading