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

Update css-to-react-native to v3.0.0 #2811

Merged
merged 1 commit into from
Oct 29, 2019
Merged

Update css-to-react-native to v3.0.0 #2811

merged 1 commit into from
Oct 29, 2019

Conversation

omBratteng
Copy link
Contributor

@omBratteng omBratteng commented Oct 16, 2019

  • Will now console.warn when omitting units on properties that require them (top, width etc.)
  • Fixed issue with percentage flex basis in flex shorthand
  • Removed support for unitless line height in font shorthand

This should close #2782

@quantizor
Copy link
Contributor

quantizor commented Oct 21, 2019

@omBratteng can you change the target branch to v5 (canary) and copy over the notes from the OP into a changelog entry?

@omBratteng omBratteng changed the base branch from master to canary October 22, 2019 08:24
@omBratteng
Copy link
Contributor Author

@probablyup I've rebased my branch to be canary and recommited the dependency update. If @jacobp100 could provide a changelog I'd add it in my PR

@jacobp100
Copy link
Contributor

Hey, the changelog is here

@quantizor quantizor added the 5.0 label Oct 22, 2019
@quantizor
Copy link
Contributor

@omBratteng just need that changelog entry with the copied notes and we're good to ship this

@omBratteng
Copy link
Contributor Author

@probablyup do you mean to add the changelog from css-to-react-native to the CHANGELOG.md file?

@quantizor
Copy link
Contributor

quantizor commented Oct 29, 2019 via email

@quantizor quantizor merged commit b44add7 into styled-components:canary Oct 29, 2019
@quantizor
Copy link
Contributor

Thanks!

@jacobp100
Copy link
Contributor

Thanks! 😄

quantizor pushed a commit that referenced this pull request Nov 7, 2019
@shamilovtim
Copy link

shamilovtim commented Dec 2, 2019

@omBratteng @probablyup @jacobp100 it's more than unitless line heights that this removed support for. this removed support for all unitless properties. this is a major breaking change in between two minor releases and it makes things difficult because there were important patches that i can't get now between the newer versions and the one that made this change. is it possible to save this for the production release? it is going to require a significant refactor and possibly a codemod.

#2782

Screen Shot 2019-12-02 at 4 26 27 PM

@quantizor
Copy link
Contributor

this is a major breaking change in between two minor releases

That is why it is being shipped on the v5 major branch not v4. You can always pin back to the prior beta until you get a chance to make relevant updates. @jacobp100 I'm not a fan of the unitless thing being a unilateral change though... your changelog made it sound like it would only affect the font shorthand.

@jacobp100
Copy link
Contributor

Will now console.warn when omitting units on properties that require them (top, width etc.)

I thought that was clear

We want to get closer to web to avoid these huge changes in the future. For example, if we ever enable the border radius transform on styled-components, that above example would suddenly break with a red-screen error

@jacobp100
Copy link
Contributor

@quantizor
Copy link
Contributor

@jacobp100 Thanks for posting about that codemod, very helpful! I also added it to the github release notes for rc0

@shamilovtim
Copy link

shamilovtim commented Dec 3, 2019 via email

@jacobp100
Copy link
Contributor

@shamilovtim There’s a lot of discussion and reasoning over why we require units over on the css-to-react-native repo. It’s worth a read

The codemod uses jscodeshift (the first step is you installing this). There does seem to be a good amount on google about getting it to run on TS files. If you want to try it out, let me know how it goes! It would be great to add docs for other people doing the same 🙂

@shamilovtim
Copy link

I created a patch for now

css-to-react-native+3.0.0.patch:

diff --git a/node_modules/css-to-react-native/index.js b/node_modules/css-to-react-native/index.js
index 0d6f377..cea064e 100644
--- a/node_modules/css-to-react-native/index.js
+++ b/node_modules/css-to-react-native/index.js
@@ -792,7 +792,6 @@ var transformRawValue = function transformRawValue(propName, value) {
 
     if (needsUnit && isNumberWithoutUnit) {
       // eslint-disable-next-line no-console
-      console.warn("Expected style \"" + propName + ": " + value + "\" to contain units");
     }
 
     if (!needsUnit && value !== '0' && !isNumberWithoutUnit) {
diff --git a/node_modules/css-to-react-native/src/index.js b/node_modules/css-to-react-native/src/index.js
index 0afce0a..351f45d 100644
--- a/node_modules/css-to-react-native/src/index.js
+++ b/node_modules/css-to-react-native/src/index.js
@@ -19,7 +19,6 @@ export const transformRawValue = (propName, value) => {
     const isNumberWithoutUnit = numberOnlyRe.test(value)
     if (needsUnit && isNumberWithoutUnit) {
       // eslint-disable-next-line no-console
-      console.warn(`Expected style "${propName}: ${value}" to contain units`)
     }
     if (!needsUnit && value !== '0' && !isNumberWithoutUnit) {
       // eslint-disable-next-line no-console

I'll try to get the codemod working once I have a clean git tree and I'll update the docs once I get it to work

@jacobp100
Copy link
Contributor

@shamilovtim
Copy link

shamilovtim commented Dec 3, 2019 via email

This was referenced Mar 10, 2020
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.

5 participants