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

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

Closed

Conversation

LucasUnplugged
Copy link
Contributor

@LucasUnplugged LucasUnplugged commented Apr 16, 2022

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
Copy link

codesandbox-ci bot commented Apr 16, 2022

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 9d62a57:

Sandbox Source
Stitches CI: CRA Configuration
Stitches CI: Next.js Configuration

@LucasUnplugged
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link

jlalmes commented Apr 19, 2022

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

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

@LucasUnplugged
Copy link
Contributor Author

LucasUnplugged commented Apr 19, 2022

@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
Copy link
Contributor Author

LucasUnplugged commented Apr 19, 2022

@jlalmes you can see what I mean here:
#650 (comment)

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
Copy link

jlalmes commented Apr 20, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 hadihallak self-requested a review June 1, 2022 13:30
@hadihallak
Copy link
Member

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 hadihallak added this to the 1.3.0 milestone Jun 3, 2022
@hadihallak
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

cinchy-m commented Jun 8, 2022

@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
Copy link
Member

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

@aromeronavia
Copy link

Any updates on this? 🙏

@LucasUnplugged
Copy link
Contributor Author

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

@aromeronavia
Copy link

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

@LucasUnplugged
Copy link
Contributor Author

@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
Copy link

Sorry for tagging 🙏

@ChristopherTrimboli
Copy link

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
Copy link
Contributor

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

@peduarte
Copy link
Contributor

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

@aroman
Copy link

aroman commented Oct 11, 2022

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
Copy link

@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
Copy link

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
Copy link

pugson commented Mar 19, 2023

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

@aromeronavia
Copy link

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

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.

None yet