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 option for label formatting #566

Merged
merged 6 commits into from
Feb 13, 2018

Conversation

sgal
Copy link
Contributor

@sgal sgal commented Feb 11, 2018

What:
New option for babel-plugin-emotion that allows to configure label format in generated class names. Implements #563

Why:
Details are in #563, but in general when debugging it is nice to have the file name in the class names (especially, for composed styles). The potential of the format is quite big. The feature and solution itself are inspired by localIdentName option in webpack's css-loader.

How:
New function that handles label generation for all cases (autoLabel, labelFormat, nothing).

Checklist:

  • Documentation
  • Tests
  • Code complete

@codecov
Copy link

codecov bot commented Feb 11, 2018

Codecov Report

Merging #566 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/babel-plugin-emotion/src/babel-utils.js 96.92% <100%> (+0.25%) ⬆️
packages/babel-plugin-emotion/src/index.js 96.92% <100%> (+0.03%) ⬆️

@tkh44
Copy link
Member

tkh44 commented Feb 11, 2018

Thank you for your contribution! I took a quick look and it looks fine, but I'd like to have @mitchellhamilton look at it as well.

@sgal
Copy link
Contributor Author

sgal commented Feb 11, 2018

@tkh44 Thanks for the feedback! I'm currently thinking about how to handle cases when label format does not include [local] cause it might result in colliding class names.

One way would be to print warning and append [local] value automatically.
Another way is to throw an error with explanation that [local] is mandatory. But something in this one rubs me the wrong way.

What do you think?

@emmatown
Copy link
Member

@sgal Collisions won't matter since there will be a hash as well and if the hash is the same then the style will be the same.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!


`string`, defaults to `null`.

This option automatically adds the `label` property (same as `autoLabel`), but allows to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should "but allows to" be "but allows you to"?

@sgal
Copy link
Contributor Author

sgal commented Feb 12, 2018

@mitchellhamilton Ah, thanks for clarifying the hashing. I fixed the phrasing in the docs.

@emmatown
Copy link
Member

I've been thinking about this more and maybe the labelFormat option should default to [local] and only insert labels when the autoLabel option is true. So rather than having two options that enable labels where one of them also lets you configure the labels, just one to enable labels and one to configure the labels.

@sgal
Copy link
Contributor Author

sgal commented Feb 12, 2018

@mitchellhamilton That does sound reasonable and I think it is easier to understand as well. I'll update the PR tonight.

@emmatown emmatown merged commit 7dc03d6 into emotion-js:master Feb 13, 2018
@matpaul
Copy link

matpaul commented Feb 24, 2018

@sgal, Hi! maybe also add path/folder or getLocalIdent function? In our project we use component/index.js, so filename - doesn't help here

@sgal
Copy link
Contributor Author

sgal commented Feb 24, 2018

@matpaul Which arguments do you think getLocalIdent function should take?

@matpaul
Copy link

matpaul commented Feb 24, 2018

@sgal
full path to file (from project root) and local for example ?

@sgal sgal deleted the feature-label-format branch November 3, 2018 16:40
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

4 participants