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

fix(Chip): avoid reassigning custom properties #3019

Merged
merged 3 commits into from
Jan 21, 2025

Conversation

mariussd
Copy link
Collaborator

Since Designsystemet is placed in a CSS layer, we can not reassign custom properties in case the consumer wants to overwrite them. Therefore we need to only assign native CSS properties, in order for them to be able to overwrite.

@mariussd mariussd self-assigned this Jan 20, 2025
Copy link

changeset-bot bot commented Jan 20, 2025

🦋 Changeset detected

Latest commit: 2118ca4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@digdir/designsystemet-css Patch
@digdir/designsystemet Patch
@digdir/designsystemet-theme Patch
@digdir/designsystemet-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jan 20, 2025

Preview deployments for this pull request:

Storybook - 21. Jan 2025 - 09:34

Copy link
Contributor

github-actions bot commented Jan 20, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 53.41% 2955 / 5532
🔵 Statements 53.41% 2955 / 5532
🔵 Functions 86.12% 180 / 209
🔵 Branches 76.69% 520 / 678
File CoverageNo changed files found.
Generated in workflow #1698 for commit 2118ca4 by the Vitest Coverage Report Action

@Barsnes
Copy link
Member

Barsnes commented Jan 20, 2025

Just to make sure we are on the same page here, is this the issue you are trying to resolve?
https://codesandbox.io/p/sandbox/3327fj

I'm not too sure if this is an issue 🤔 It comes down to how you look at it:

  • Either we have a lot of css vars, and the user has to overwrite a lot of them.
    • This makes it cumbersome to make sure every var is updated to the correct style a user wants
  • We have 1 css var that we overwrite using state selectors, i.e. `[data-variant="secondary"]
    • The consumer needs to select the state they want to override vars for

I don't think this is a clear cut issue, and I think it needs at least some discussion..

Feel free to add comments directly to this PR.

@eirikbacker
Copy link
Contributor

I think we need to huddle to make sure we all understand the issue correctly ☺️

@Barsnes
Copy link
Member

Barsnes commented Jan 20, 2025

I think we need to huddle to make sure we all understand the issue correctly ☺️

Sure, but this also needs to be documented properly, and understandable, on github

Copy link
Member

@Barsnes Barsnes left a comment

Choose a reason for hiding this comment

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

Only requesting changes so we don't accidentaly merge this without having the discussion tomorrow

@mariussd
Copy link
Collaborator Author

mariussd commented Jan 20, 2025

@Barsnes The issue is that if I overwrite --dsc-chip-background in my own code, it will always take presedence over reassignments in the CSS file from Designsystemet, because the Designsystemet-layer has a lower priority.

An example

In my project:

@layer my.layer {
  .chip {
    composes: ds-chip from "@digdir/designsystemet-css";

    --dsc-chip-background: transparent;
}

If I do this, the reassignment of --dsc-chip-background that's used to handle hover and active states in Designsystemet won't work, as the reassignment in my project always takes presedence.

In designsystemet/packages/css/src/chip.css, line 98–108:

  ...

  @media (hover: hover) and (pointer: fine) {
    &:not(:has(:disabled, [aria-disabled='true'])) {
      &:hover {
        --dsc-chip-background: var(--dsc-chip-background--hover);
      }

      &:active {
        --dsc-chip-background: var(--dsc-chip-background--active);
      }
    }
  }

I can get around this by copying the code to my project, and reassign every custom property in the Designsystemet CSS file, basically re-implementing the Designsystemet CSS file line by line. However, I don't see this as scalable. Any change to this CSS file in Designsystemet could be a breaking change to my implementation. That would be very tedious. Luckily, it's solvable by assigning the custom property values to native CSS properties, instead of reassigning the custom properties themselves.

This example can be solved by writing it like this, as my PR suggests:

  ...

  @media (hover: hover) and (pointer: fine) {
    &:not(:has(:disabled, [aria-disabled='true'])) {
      &:hover {
        background: var(--dsc-chip-background--hover);
      }

      &:active {
        background: var(--dsc-chip-background--active);
      }

      ...
    }
  }

@Barsnes
Copy link
Member

Barsnes commented Jan 20, 2025

Right, so just like my codesandbox?

@mariussd
Copy link
Collaborator Author

@Barsnes Sorry, unfortunately I don't understand your explanation in the codesandbox 😅 Can you explain in more detail?

@mimarz
Copy link
Collaborator

mimarz commented Jan 21, 2025

We'll make separate issue on how to solve variants and override styling with CSS variables.

@eirikbacker eirikbacker requested a review from Barsnes January 21, 2025 08:32
@mimarz mimarz merged commit 0f8814b into next Jan 21, 2025
10 checks passed
@mimarz mimarz deleted the fix/chip-custom-property-reassignment-cleanup branch January 21, 2025 08:45
mimarz pushed a commit that referenced this pull request Jan 21, 2025
Since Designsystemet is placed in a CSS layer, we can not reassign
custom properties in case the consumer wants to overwrite them.
Therefore we need to only assign native CSS properties, in order for
them to be able to overwrite.

---------

Co-authored-by: Eirik Backer <eirik.backer@gmail.com>
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.

4 participants