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

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

Open
2 tasks done
zoonun opened this issue Aug 28, 2024 · 2 comments
Open
2 tasks done

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

zoonun opened this issue Aug 28, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@zoonun
Copy link

zoonun commented Aug 28, 2024

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

  • I've made research efforts and searched the documentation
  • I've searched for existing issues
@zoonun zoonun added the bug Something isn't working label Aug 28, 2024
@pnodet
Copy link

pnodet commented Aug 29, 2024

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

@joseph0926
Copy link

joseph0926 commented Sep 1, 2024

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)

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

4 participants
@pnodet @zoonun @joseph0926 and others