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

feat: formatClassName, formatTokenName, and formatCssVar #1848

Closed
wants to merge 30 commits into from

Conversation

ivanbanov
Copy link
Contributor

@ivanbanov ivanbanov commented Dec 18, 2023

📝 Description

Migrating a large codebase from Stitches to PandaCSS imposed some challenges due to the naming convention differences between the libraries. The main complication comes from the $ prefix in Stitches, the workaround suggested by PandaCSS is simply adding a $ sign in the token name, but it causes some problems.

  • The CSS var is also generated with the dollar sign, and there is no way to dasherize it to avoid breaking the current CSS vars that are added by stitches e.g
    • PandaCSS --colors-\$red.100
    • Stitches --colors-red-100
  • The final class name is not optimal with the inflict dollar sign. eg. .text_\$red.100
  • The token names can not be customized making it harder to migrate from a codebase using purely - or _ for example in the css vars. It would be nice to have an option to modify how the final token name is generated.
    • PandaCSS red.100
    • Stitches $red-100

⛳️ Current behavior

It's not possible to customize the token names, class names, and cssvars

🚀 New behavior

  • Added a new config formatTokenName
    This config receives the token path and returns a string with the new token name

  • Added a new config formatClassName
    This config receives the token as a string (from the formatTokenName) and returns a string that will be used as <prefix>-<formatted classname>. Hashed class names are not affected

  • Added a new config formatCssVar
    This config receives the token path and returns a formatted name. Hashed class names are not affected

all together

const dasherize = (token) => token
 .toString()
 .replace(/[^a-zA-Z0-9]+/g, '-')
 .replace(/^-+|-+$/g, '')

export default defineConfig({
  // Stitches preset
  separator: '-',
  formatTokenName: (path) => `$${path.join('-')}`,
  formatClassName: dasherize,
  formatCssVar: dasherize,
})

💣 Is this a breaking change (Yes/No):

No

Copy link

changeset-bot bot commented Dec 18, 2023

⚠️ No Changeset found

Latest commit: b615a2e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link

vercel bot commented Dec 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
panda-docs ✅ Ready (Inspect) Visit Preview Jan 17, 2024 5:02pm
panda-playground ✅ Ready (Inspect) Visit Preview Jan 17, 2024 5:02pm
panda-studio ✅ Ready (Inspect) Visit Preview Jan 17, 2024 5:02pm

Copy link

vercel bot commented Dec 18, 2023

@ivanbanov is attempting to deploy a commit to the Chakra UI Team on Vercel.

A member of the Team first needs to authorize it.

@ivanbanov
Copy link
Contributor Author

Hey @astahmer @segunadebayo let me know if there is anything I could do to get this one merged :)

@anubra266
Copy link
Collaborator

anubra266 commented Jan 17, 2024

Found a bug @ivanbanov when I set prefix in the config, the separator is automatically set to _ meanwhile the default separator in panda is -

This is not happening in the generated css, but in the generated classNames

CleanShot 2024-01-17 at 08 37 57@2x

@ivanbanov
Copy link
Contributor Author

Hey @anubra266 Im not sure Im following 🤔 the default separator from Panda is actually _

While switching the configs I also got stuck in the state that you show in the image, but after rebuilding everything was properly built, Im wondering if it was a glitch in the autoreload

@segunadebayo
Copy link
Member

segunadebayo commented Jan 22, 2024

Thanks for working on this @ivanbanov.

@astahmer and I have discussed this PR at least 3-4 times this past week. Here are some potential pitfalls we found with this PR.

formatClassName has the potential to break the generated CSS and className.

Given the following panda config:

const dasherize = (token) => token
 .toString()
 .replace(/[^a-zA-Z0-9]+/g, '-')
 .replace(/^-+|-+$/g, '')

export default defineConfig({
  // Stitches preset
  separator: '-',
  formatTokenName: (path) => `$${path.join('-')}`,
  formatClassName: dasherize,
  formatCssVar: dasherize,
})

These use cases will lead to incorrect class names.

css({ content: 'Price: $25.99' }) // the dollar will be altered
// => content-Price-25-99

css({ content: 'Price: 25.99' }) // the same className will be emitted leading to conflicts
// => content-Price-25-99

For the Stitches example, this might lead to className conflicts and style issues since you can't predict all css style declarations. Allowing users to customize the className formatting can be much worse depending on the function passed.

formatTokenName and formatCssVar looks good. Without an option to customize the className, the value of those functions aren't so much.

The recommended approach we agreed on is to replace these functions with a single tokenNamePrefix config property might be more fool-proof, allowing us to manage the nuances internally.

export default defineConfig({
  tokenNamePrefix: '$'
})

With this approach, we would get most of the features requested. The only downside is that the className would still include the "$" prefix, something like, bg_$red.200.

To get the className to look something like bg_$red200, we recommend flattening the tokens. See the Stitches recommendation as well

// before
const colors = {
   red: {
      200: { value: '<value>' }
   }
}

// after
const colors = {
    red200: { value: '<value>' }
}

What do you think of this approach?

Once agreed, this new feature will be shipped before v1

@ivanbanov
Copy link
Contributor Author

hey @segunadebayo

when I first added the formatClassName I also thought about that but I preferred to go for the trade-off "if you format it, you are responsible for not breaking it", and although the example you shared is a possible clash I wonder how much it will be an issue in the real world.

For sure, adding tokenNamePrefix is already a big win, but if I understand correctly it would replace the formatTokenName and formatCssVar options too (which are used to dasherize in my example), right?

Just dropping other ideas that I had to walk around it

  • throw an error when a class clashes with another one
  • instead of instead of receiving a function in the formatters, only allow some predefined options like dash | camelCase | escape and then use the tokenNamePrefix (but I wouldn't add the prefix to the final css and classname)
    e.g.
// config
{
  theme: {
    colors: {
      red: { 200: { value: '<value>' } }
    }
  },
  tokenNamePrefix: '$',
  tokenFormat: 'dash' 
}

css({ color: '$red-200' })

/**
 * classname: .text_red-200
 * cssvar:    --var(--colors-red-200)
 * token:     $red-200
 */

would it be a possible middle-term in the approaches?

@segunadebayo
Copy link
Member

Good points @ivanbanov.

The goal of the CSS function is to join the property and value with _; it doesn't (and probably shouldn't) have knowledge of the token and token name prefix to keep the runtime fast.

Not adding the prefix to the final class name is primarily an aesthetic concern; I don't assume it'll be a deal breaker for most users. I recommend using the hash: true to make the classNames terser.

The tokenFormat sounds interesting. What would the other options look like? I mean the 'camelCase' and escape options.

@ivanbanov
Copy link
Contributor Author

ivanbanov commented Jan 23, 2024

@segunadebayo Actually before I started implementing the cssVar formatter as a function I first tried the predefined approach, which worked as expected, you can see it here. I even thought it would be possible to do the same with the className and tokenName, but instead I went for the 100% customization.

Going in this direction I'd say that we could have the following API

theme: {
  tokens: {
    spacing: {
      xLarge: { value: '12px' }
    }
  },
  tokenNamePrefix: '$',
  tokenFormat: <'dash' | 'camelCase' | 'snakeCase' | 'escaped'>
}


/**
 * dash
 * classname: .m_x-large
 * cssvar:    --var(--spacing-x-large)
 * token:     $x-large
 */

/**
 * camelCase
 * classname: .m_xLarge
 * cssvar:    --var(--spacingXLarge)
 * token:     $xLarge
 */

/**
 * snakeCase
 * classname: .m_x_large
 * cssvar:    --var(--spacing_x_large)
 * token:     $x_large
 */

/**
 * escaped
 * classname: .m_\$xLarge
 * cssvar:    --var(--spacing-\$xLarge)
 * token:     $xLarge
 */

// -- edited (adding colors nested example too)

theme: {
  tokens: {
    colors: {
      red: {
        100: { value: '<value>' }
      }
    }
  },
  tokenNamePrefix: '$',
  tokenFormat: <'dash' | 'camelCase' | 'snakeCase' | 'escaped'>
}


/**
 * dash
 * classname: .text_red-100
 * cssvar:    --var(--colors-red-100)
 * token:     $red-100
 */

/**
 * camelCase
 * classname: .text_red100
 * cssvar:    --var(--colors-red100)
 * token:     $red100
 */

/**
 * snakeCase
 * classname: .text_red_100
 * cssvar:    --var(--colors-red_100)
 * token:     $x_large
 */

/**
 * escaped
 * classname: .text_\$red.100
 * cssvar:    --var(--colors-\$red.100)
 * token:     $red.100
 */

It would still achieve my main goal, and give less flexibility to the final user, which can be good to avoid class name clashes, how does it sound to you?

@segunadebayo
Copy link
Member

segunadebayo commented Jan 24, 2024

Sounds good to me @ivanbanov

The className might not ship with this. I don't see a clean way to solve it because requires the css function to have knowledge of properties that could use token. We shouldn't do that.

If we consider another token category, say spacing, there should be a semantic difference between "using a token" or not.

css({ margin: "$40" })
// class: m_$40
// css: margin: var(--spacing-40)

css({ margin: "40" })
// class: m_40
// css: margin: 40px

Customizing the className will be problematic in this use case.

If we're going to move forward with this, I'd recommend scoping this down to configuring the token name and format.

@ivanbanov
Copy link
Contributor Author

@segunadebayo I see it, the formatCssVar and formatTokenName are only connected to the tokens while the formatClassName is more flexible and has to "understand" arbitrary values.

Im fine removing it, do you think that the other formatters are in good shape to make their way into the v1? Which direction do you want to take from here?

astahmer added a commit that referenced this pull request Jan 27, 2024
astahmer added a commit that referenced this pull request Jan 27, 2024
segunadebayo added a commit that referenced this pull request Feb 1, 2024
* refactor: token dictionary

* feat: hooks "tokens:created"
test: allow passing custom hooks

refactor: dict.formatTokenName
refactor: use array of string as path rather than dot-delimited string for colorPalette
-> so that the formatTokenName can be replaced with custom logic with hooks, we couldnt before because the join('.') logic was hardcoded everywhere

* feat: dictionary.formatCssVar

* feat: utility.toHash + 'utility:created' hook with setToHashFn

* feat: 'codegen:prepare' hook

* test: port those from #1848

* chore: add changeset

* chore: update changeset

* refactor: code

* chore: remove semantic token strictness

* test: update snaps

* refactor: rename methid

* chore: update

* refactor: config resolved hook

---------

Co-authored-by: Segun Adebayo <[email protected]>
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.

3 participants