-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Relax requirement for label extraction - containing functions dont have to be PascalCased anymore #1805
Conversation
🦋 Changeset is good to goLatest commit: 9bddf18 We got this. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 9bddf18:
|
e51e5b7
to
61d7160
Compare
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is good - labels are largely intended to be a way to find which component corresponds to which class name and this means if you have a helper function, it would show up all over the place and therefore make labels less helpful
This only gives a label based on a function if there is no intermediate declaration, etc, in the ancestors' chain. Right now this gets a label: const foo = css`color: green;` but this doesnt const foo = () => css`color: green;` This is surprising for some (had a couple of questions regarding this on Slack and here on GitHub). I don't see much difference between a "partial" styles (exhibit one) and a helper function creating such (exhibit two). Could you maybe prepare a case where you think this would become problematic? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay sure. Could you check that that any valid variable name is also valid in a class name or otherwise, remove the bad characters or something? Other than that, LGTM
…ve to be PascalCased anymore
61d7160
to
9bddf18
Compare
@mitchellhamilton all "locals" are being sanitized when actually generating labels:
|
…ve to be PascalCased anymore (emotion-js#1805)
It IMHO makes sense to relax this, otherwise styles from functions calling
css
dont receive labels.