shadcn-ui / ui

Beautifully designed components that you can copy and paste into your apps. Accessible. Customizable. Open Source.
https://ui.shadcn.com
MIT License
69.37k stars 4.13k forks source link

[bug]: Duplicate classNames(border) in checkbox, radio group #4654

Open zoonun opened 2 weeks ago

zoonun commented 2 weeks ago

Describe the bug

If I install checkbox in CLI, I encountered duplicate border-color issue.

image

PR#1089 said that this problem has fixed... I found accurate comment on related first bug issue, and I checked Input, Alert, TextArea, Select's bug was fixed. But I still encountered same error when I install checkbox, radio group.

transform-css-vars.ts's applyColorMapping: function - border => border border-border may cause problem, but I don't know what border-border is... There's anyone knows about what border-border means? 😢

I created this issue because the issue was closed even though the bug was not resolved perfectly.

Affected component/components

Checkbox, Radio Group

How to reproduce

  1. enter pnpm dlx shadcn-ui@latest add checkbox or radio-group on your project.
  2. go to ui/checkbox...ui/radio-group
  3. you'll see the error.

Codesandbox/StackBlitz link

No response

Logs

No response

System Info

Mac M1 Pro Max, Latest Chrome

Before submitting

pnodet commented 2 weeks ago

This has been annoying for so long… Already expressed in #2424 and #2882

joseph0926 commented 2 weeks ago

The problem seems to start with not selecting css var in the initial shadcn@latest init options

Since css var is undefined, shacn does the conversion internally via the applyColorMapping function, which is currently set to

if (input.includes(” border ”)) { 
    input = input.replace(” border ‘, ’ border border-border ”)
  }

There is special handling for borders, like in this code So what would have been border border-xxx in the original code becomes border border border-border border-xxx Instead, in the following code

 lightMode.add(
        [variant, `${prefix}${mapping.light[needle]}`]
          .filter(Boolean)
          .join(“:”) + (modifier ? `/${modifier}` : “”)
      )

Although there is logic to remove duplicate code, border-primary and border-boder are different on conversion as neutral-900, neutral-200 (based on neutral, light) This would be fine if this happened wherever there was a border, but for inputs with border attributes, the

“border": “neutral-200”,
      “input": “neutral-200”,

Since they are the same, “border-boder” and “border-input” are duplicated by “border-neutral-200”, so one is removed and the problem does not appear

Conclusion.

  1. css variables were not set at the initial setup, so the internal conversion process did not handle border perfectly.
  2. solution is to set css variables on initialization or,
    if (input.includes(“ border ‘) && !input.includes(’border-primary”)) { }
    input = input.replace(” border ‘, ’ border border-border ”)
    }

    I think we should ask the @shadcn team to do something about border-primary in this way (I don't know if this is a perfect solution, but it passed my tests for checkboxes)


Below is the test code for a “checkbox” that I arbitrarily created

import { describe, expect, it } from "vitest"

import { applyColorMapping } from "../utils/transformers/transform-css-vars"
import neutral from "./neutral.json"

describe("applyColorMapping transformation", () => {
  const realMapping = neutral.inlineColors

  it("should correctly transform class names with color mapping", () => {
    const input =
      "peer h-4 w-4 shrink-0 rounded-sm border border-primary ring-offset-background focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:bg-primary data-[state=checked]:text-primary-foreground"

    const expectedOutput =
      "peer h-4 w-4 shrink-0 rounded-sm border border-neutral-900 ring-offset-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-neutral-400 focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:bg-neutral-900 data-[state=checked]:text-neutral-50 dark:border-neutral-50 dark:ring-offset-neutral-950 dark:focus-visible:ring-neutral-800 dark:data-[state=checked]:bg-neutral-50 dark:data-[state=checked]:text-neutral-900"

    const result = applyColorMapping(input, realMapping)

    expect(result).toBe(expectedOutput)
  })
})

The result is shown below

  1. When the code is unmodified,
스크린샷 2024-09-01 오전 11 07 46
  1. When modified the code,
스크린샷 2024-09-01 오전 11 09 07

---. (sorry for the awkward wording due to the translator)