Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import of selector fails when it hasn't any base styles #213

Open
Mad-Kat opened this issue Nov 19, 2024 · 1 comment
Open

Import of selector fails when it hasn't any base styles #213

Mad-Kat opened this issue Nov 19, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@Mad-Kat
Copy link
Collaborator

Mad-Kat commented Nov 19, 2024

When importing components as selectors, we always attempt to generate an import for the component's base class, which causes issues with components that only have dynamic classes based on props.

Currently

When using an imported component as a selector:

import { Label } from "./other";

const OverwriteLabel = styled.div`
  ${Label}{
    color: pink;
  }
`

we generate:

const OverwriteLabel = /*YAK Extracted CSS:
.OverwriteLabel {
  --yak-css-import: url("./other:Label",selector) {
    color: pink;
  }
}
*/ /*#__PURE__*/ styled.div(__styleYak.OverwriteLabel);

which results in this CSS:

.OverwriteLabel {
  :global(.other_Label__l1FMk) {
    color: pink;
  }
}

Problem

The generated CSS assumes there's a base class named Label in the imported component's CSS. However, this assumption fails when the imported component only has dynamic classes based on props:

export const Label = styled.label<{ $variant: "primary" | "secondary" }>`
  ${({ $variant }) => $variant === "primary" ? css`
    color: red;
  ` : css`
    color: blue;
  `}
`;

which results in this CSS:

.Label__ {
  color: red;
}
.Label__1 {
  color: blue;
}

In this case, there is no base .Label class to target, causing the selector to fail.

Proposed Solutions

  1. Generate Base Class Always
    Always generate a base class for components, regardless of whether they have static styles
    Pros: Maintains backwards compatibility
    Cons: Additional CSS overhead

  2. Throw Descriptive Error
    Detect when a component lacks a base class and throw an error with an explanation of the issue and suggestions for solutions like "add base styles to the component" or "split into separate components"
    Pros: Clearer developer experience
    Cons: Breaking change for the migration from styled-components to next-yak

@jantimon
Copy link
Owner

thanks for the report - I think we should simply always generate a base class

the overhead for the html and css is rather tiny because there are probably just a limited cases where we wouldn't use a base class

because of this issue I came up with an idea for a potential future default case optimisation - take this example:

const Button = styled.button<{$active: boolean}>`
  ${({$active}) => $active 
    ? css`color: red;` 
    : css`color: blue;`
  }
`;

Instead of three classes:

.Button-abc123 {}
.Button-abc123--active { color: red; }
.Button-abc123--inactive { color: blue; }

We could optimize to:

.Button-abc123 { color: blue; }
.Button-abc123--active { color: red; }

but let's solve the immediate issue first with a simple fix

@jantimon jantimon added the bug Something isn't working label Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants