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

Variant classes aren't escaped as expected #731

Closed
joe-bell opened this issue Aug 23, 2021 · 4 comments
Closed

Variant classes aren't escaped as expected #731

joe-bell opened this issue Aug 23, 2021 · 4 comments

Comments

@joe-bell
Copy link

Bug report

Describe the bug

When trying to create variants with characters that need escaping (e.g. a file path), the outputted class wasn't escaped.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Go to https://codesandbox.io/s/stitches-issue-escaped-classes-toqfg
  2. Scroll down to the instances of textWithPathVariant and TextWithPathVariant which are used to demonstrate the issue
  3. See how the text isn't displaying as red
  4. When viewing the style in the devtools, the variant's class isn't escaped (i.e. / should be transformed to \/ and . should be transformed to \.

Expected behavior

I would expect the outputted CSS class for the variant to be escaped, with the HTML class unescaped

Screenshots

N/A

System information

  • OS: macOS
  • Browser (if applies) Chrome
  • Version of Stitches: 1.0.0-canary-15 and 0.2.5
  • Version of Node.js: 14.7.5

Additional context

To get around this in the meantime, I'm sanitising the variant string of the offending characters

const strip = (str: string) => str.replace(/[\/]|[.]/g, "-");

but it would be great to have this working out of the box 🙏

Perhaps Modulz could utilise the CSS.escape() polyfil?

@jonathantneal
Copy link
Contributor

I agree that we should add this, but I’m not sure if now is the right time. We currently rely on some of this looseness for escape hatches.

Now, allow me to play out a hypothetical conversation —

I might suggest you use CSS.escape. You might reply that Node does not support this. Then I might lament that the client should not pay the decompression or computational burden for Node, and you might reply that they already do for CSSOM. Then we might agree to split the code base so that only Node gets the ponyfills for CSSOM and CSS escape. Finally, I might plead with you to let us wait until version one is stable.

Stitches may never be done — but it will keep getting better.

@jonathantneal
Copy link
Contributor

Our current thinking is to put this burden on the developer who wants to use special characters, rather than move it into Stitches, especially since the cost depends on whether the code will execute either within a Browser, within Node, or a combination of either of those.

@joe-bell
Copy link
Author

Sounds sensible, thanks for the update!

@YarnSphere
Copy link

Hey, sorry to bump this old issue, but I'd like to request that this be reconsidered.

I have use-cases for variants with slashes and dots in their names such as (inspired by Tailwind):

const Box = styled("div", {
  variants: {
    width: {
      "1/2": { width: "50%" },
      "1/3": { width: "33.333333%" },
      "2/3": { width: "66.666667%" },
      "full": { width: "100%" }
    }
  }
});

In order to make it possible to write something like: <Box width="1/3" />.

I can't find any acceptable solution from my end that doesn't make me think "why am I even trying to use variants for this". Even though I think that this should be a valid use-case for variants.

I feel like Stitches emitting invalid CSS should be seen as a bug rather than a feature.

@jonathantneal, I'd be interested in hearing about how you're relying on this "looseness for escape hatches", if possible. It might be possible to implement some kind of escaping (not relying on CSS.escape) that could escape some common likely-to-be-used characters such as . or / while allowing others to pass through for an escape hatch.

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

No branches or pull requests

3 participants