stitchesjs / stitches

[Not Actively Maintained] CSS-in-JS with near-zero runtime, SSR, multi-variant support, and a best-in-class developer experience.
https://stitches.dev
MIT License
7.75k stars 253 forks source link

Add support for custom (human-friendly) class names #1005

Closed LucasUnplugged closed 2 years ago

LucasUnplugged commented 2 years ago

Taking a stab at a different API to this, based on feedback in #916 (see details there).

(Addresses #650)

Proposed API

Users can use a withName utility method to pass in a string as the custom component name, with their calls to styled, like so:

// Renders as `c-Label-sOm3h4Sh`
const Label = styled.withName("Label")("label", {...})

This proposed solution could be combined with a babel plugin, to make friendly class names possible with no extra effort, for those using Babel.

The existing syntax would be unchanged, plus this approach works just as well with css.withName("Xyz")(...), so it's not limited to React only.


I feel that this functionality makes debugging much, much easier, so would love to work with the core team to find the right API to get this into production ❤️ .

codesandbox-ci[bot] commented 2 years ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9d62a578da2eab1367b48198e9de5297abbadf8c:

Sandbox Source
Stitches CI: CRA Configuration
Stitches CI: Next.js Configuration
LucasUnplugged commented 2 years ago

@hadihallak what do you think about the proposed APIs I mentioned above?

The underlying implementation still passes a parameter to styled under the hood, but I don't think it's possible to avoid that without breaking changes and/or a major rewrite of this section of the codebase.

That said, I think these 2 options can nicely encapsulate the API, and make for really clean documentation, especially the styled.withName("Xyz")(...) syntax.

I toyed with making it something like the following, but it seemed more cumbersome, with regards to server-side supoort.

styled(...).named("Xyz")

I was able to do it by regenerating the underlying cssComponent, but that felt really wasteful, performance-wise.

LucasUnplugged commented 2 years ago

I've pushed an update that no longer pollutes the existing API at all, and works the exact same way for non-React usage (css.withName("Xyz")(...)). I also added some more tests.

@hadihallak let me know what you think of this approach; since it doesn't pollute the current API, is consistent, and is really well encapsulated, I think/hope it addresses your previous concerns.

It should be pretty easy to use this with Babel to make it automatic for folks with that setup. I can look at updating the Babel plugin I had built, if this gets merged.

jlalmes commented 2 years ago

@LucasUnplugged - is this not already possible with the prefix option?

const { styled, ... } = createStitches({ prefix: "xyz" })
LucasUnplugged commented 2 years ago

@LucasUnplugged - is this not already possible with the prefix option?


const { styled, ... } = createStitches({ prefix: "xyz" })

Not quite; the prefix is the same in every Stitches style in your entire codebase — or at least for each instance of createStitches.

This lets you define a unique "prefix" for class names on a per-component basis, which is especially useful when you use composition, because you wouldn't know if the rendered style came from your child component, or one of the parent components you composed.

LucasUnplugged commented 2 years ago

@jlalmes you can see what I mean here: https://github.com/modulz/stitches/issues/650#issuecomment-887131413

Something like this:

<header class="c-Box-lesPJm c-Flex-dhzjXW c-Row-lfylVv c-Header-fVzExb"><h1>Stitches Demo</h1></header>

<button class="c-Box-lesPJm c-Button-bItMgZ c-Button-bItMgZ-liHaWo-primary-true">Primary</button>
jlalmes commented 2 years ago

Ok understood, in this case should we be creating an opt-in babel plugin to do this just like styled-components? https://styled-components.com/docs/tooling

LucasUnplugged commented 2 years ago

Ok understood, in this case should we be creating an opt-in babel plugin to do this just like styled-components? https://styled-components.com/docs/tooling

Yes, I have a published (npm) package of a babel plugin for the first version of this I did; however, the core team didn't like that version's API, so I would like to wait for confirmation that this will be approved before I invest time in updating that plugin.

I'm pretty sure I can just reuse that same plugin, but I have to look at it again, once this is approved.

LucasUnplugged commented 2 years ago

Ok understood, in this case should we be creating an opt-in babel plugin to do this just like styled-components? https://styled-components.com/docs/tooling

Yes, I have a published (npm) package of a babel plugin for the first version of this I did; however, the core team didn't like that version's API, so I would like to wait for confirmation that this will be approved before I invest time in updating that plugin.

I'm pretty sure I can just reuse that same plugin, but I have to look at it again, once this is approved.

@jlalmes This is the plugin I was talking about; it only worked with my fork, which the core team never agreed to merge (they didn't like the API I chose). If this gets merged, I'll update it to work with this.

hadihallak commented 2 years ago

Hey @LucasUnplugged Good news! we're gonna move on with this feature. will be thinking about the API a bit more to make sure that it inline with any future plans and will submit a review on Monday.

Thank you so much for working on this and for your patience 🙏

hadihallak commented 2 years ago

I gave the API thought and i think we're gonna go with an API similar to styled-components with the withConfig function because that's a quite familiar API to a lot of the css in js libraries.

So for this feature and for similarity I think we're gonna go with an API that looks like this:

styled.withConfig({displayName: 'something'})('button', {...styles})

Internally this is what would happen: 1- we set the display name on the component 2- in dev mode we will make the class name friendly like in your PR but based on the displayName

I will implement the base for the withConfig API this week to implement the componentId option so you don't need to update this PR for now

The final API might change slightly from the example that I provided (still not sure whether we want withConfig to be a method on the styled function or the component itself

Keen to hear your thoughts and ideas.

LucasUnplugged commented 2 years ago

So for this feature and for similarity I think we're gonna go with an API that looks like this:

styled.withConfig({displayName: 'something'})('button', {...styles})

Internally this is what would happen:

  1. we set the display name on the component
  2. in dev mode we will make the class name friendly like in your PR but based on the displayName

That sounds good to me. Most of the need for this feature is in dev anyway, and I'm all for sticking with conventions.

cinchy-m commented 2 years ago

@hadihallak Given your example references styled, does that mean this would only be available in the react package and not core?

It would be great if this was also made available for css too.

hadihallak commented 2 years ago

@cinchy-m I was just giving an example. the final API should support both the styled and css functions

aromeronavia commented 2 years ago

Any updates on this? 🙏

LucasUnplugged commented 2 years ago

Closing this PR, since this code as-is won't be the final version.

aromeronavia commented 2 years ago

@LucasUnplugged is there gonna be a follow up soon on this?

LucasUnplugged commented 2 years ago

@LucasUnplugged is there gonna be a follow up soon on this?

Oh, it's not up to me: I'm not part of the Stitches team.

aromeronavia commented 2 years ago

Sorry for tagging 🙏

ChristopherTrimboli commented 2 years ago

I've been reading all the back and forth on this feature request. It's a big enough requirement for some of us to ditch Stitches for styled-components. The dev debugging experience with classNames is much easier in styled-components.

I am also disappointed with the amount of roundabout and stalling the stitches team is making, when multiple people have submitted functional PRs.

Why has this been discussed for 2 years and not shipped?

peduarte commented 2 years ago

this would be great. any chance we can get this in? @hadihallak

peduarte commented 2 years ago

happy to talk over API choices etc. but ultimately it'll be a huge improvement in DX.

aroman commented 1 year ago

i'm new to stitches, and was very surprised that this feature is not included — especially given the mention on the webpage about "best in class DX". this feels like tablestakes DX, and unfortunately it means i'll have to pass on this library, which is a shame because there's a lot to love :(

cleferman commented 1 year ago

@peduarte I tagged you because you're the last contributor to reply here. What's the state of this feature? I wanted to try out stitches but I won't be using it since friendly class names are not automatically generated.

How do you guys actually find which styled component applies the css to your html? Generally, when I have a visual issue in my app I inspect the DOM and look at the element that is rendered. If I don't have a friendly class name (e.g: the name of the styled component) then I don't know where to look in my code to modify something.

So I'd be happy to give stitches another go if I have a way to quickly identify my component, this doesn't need to be accomplished through friendly class names, just to have a way to identify it.

bartcheers commented 1 year ago

Closing the loop here: withConfig has been introduced after this issue was closed, achieving the desired behavior. It's still missing in the docs, but instructions can be found here.

pugson commented 1 year ago

Awesome. Now a babel plugin can be made that automatically does this for all Stitches components in dev based on their name.

aromeronavia commented 1 year ago

@pugson that was the original @LucasUnplugged proposal but got rejected by the core team