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

Added minify option to createGenerateId #1075

Merged
merged 13 commits into from
May 21, 2019

Conversation

HenriBeck
Copy link
Member

What would you like to add/fix?
This adds an option to the createGenerateId to allow to opt-in into minification of class names.

Corresponding issue (if exists): #1072

@HenriBeck HenriBeck requested a review from kof March 28, 2019 21:41
disableStylesGeneration?: boolean,
media?: string,
children: Node
interface Props {
Copy link
Member

Choose a reason for hiding this comment

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

why do you use in this case and in case of CreateGenerateIdOptions an interface instead of type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just being used to use interfaces because of TS

Copy link
Member

Choose a reason for hiding this comment

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

I think even in ts interfaces have a bit different purpose than types., we should use type when possible and interface when needed.


### Improvements

- [jss] Add option for opt-in minification of class names. ([#1075](https://github.com/cssinjs/jss/pull/1075))
Copy link
Member

Choose a reason for hiding this comment

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

I think it's a feature and a breaking change

@kof
Copy link
Member

kof commented Apr 10, 2019

we also need to update the docs

Copy link
Member

@kof kof left a comment

Choose a reason for hiding this comment

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

let's go!

P.S. I would change interface to a type, but I am no expert in TS.

@kof
Copy link
Member

kof commented May 8, 2019

Just noticed we still didn't merge this one

@HenriBeck
Copy link
Member Author

I will finish this tomorrow evening


return nativeEscape ? nativeEscape(str) : str.replace(escapeRegex, '\\$1')
}
export default str => (nativeEscape ? nativeEscape(str) : str.replace(escapeRegex, '\\$1'))
Copy link
Member Author

Choose a reason for hiding this comment

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

We will always need to escape the string now because the user might not minify the string

We could also pass here the sheet options and check if the user wants to minify the selector

@HenriBeck HenriBeck merged commit d53092c into master May 21, 2019
bhupinderbola pushed a commit to bhupinderbola/jss that referenced this pull request Sep 17, 2019
* Added minify option to createGenerateId

* Update changelog

* Update TS types

* Update changelog

* Update size snapshot

* Update size snapshots

* Added docs

* Fix linting

* Remove check for env production in react jss

* Update size snapshot

* Fix tests
@iamstarkov iamstarkov deleted the jss/feature/add-minify-id-option branch October 4, 2019 09:42
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

2 participants