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

chore(lint): Add stylelint script #652

Merged
merged 4 commits into from
Dec 5, 2017

Conversation

chinesedfan
Copy link
Member

As discussed in #604, I integrated stylelint by following styled-components tutorial.

Please notice that even with the official stylelint-processor-styled-components, stylelint is not 100% compatible with styled-components. So I added some customized rules.

  "rules": {
    // 1
    "block-no-empty": null,
    // Inspired by https://github.com/styled-components/css-to-react-native/blob/master/src/tokenTypes.js#L31
    // to avoid unitless shorthands properties crash
    "declaration-property-value-whitelist": {
      "margin": ["/^(0|[+-]?(\\d*\\.)?\\d+px)( (0|[+-]?(\\d*\\.)?\\d+px))?$/"],
      "padding": ["/^(0|[+-]?(\\d*\\.)?\\d+px)( (0|[+-]?(\\d*\\.)?\\d+px))?$/"],
    },
    // 2
    "declaration-colon-newline-after": null,
    "value-list-max-empty-lines": null
  },
const PostButtonIcon = styled(Icon).attrs({
  color: props => (props.disabled ? colors.grey : colors.primaryDark),
})``; // here will generate an empty block
const BadgeText = styled.Text`
  ${{ ...fonts.fontPrimaryBold }};
  background-color: transparent;
  ${({ largeText }) =>
    `font-size: ${largeText ? normalize(9.5) : normalize(7)};`};
`;

It will be parsed as,

const BadgeText = styled.Text`
  ${{ ...fonts.fontPrimaryBold }};
  background-color: transparent;
  -styled-mixin1: dummyValue
/* dummyComment */;
`;

@coveralls
Copy link

Coverage Status

Coverage remained the same at 39.937% when pulling cc4c29a on chinesedfan:add_stylelint into 2bc73fe on gitpoint:master.

@machour
Copy link
Member

machour commented Dec 3, 2017

Thanks for looking into this! For the second case, would formatting the rule as you suggested in #648 solve the compatibility issue?

@chinesedfan
Copy link
Member Author

@machour No, rules in case 2 will only affect multi-lines declarations. And I replied with my understandings of those 2 styles in #648 directly.

Copy link
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

👍

@machour
Copy link
Member

machour commented Dec 4, 2017

Applied this PR diff on my macbook, got the following error message:
screen shot 2017-12-04 at 2 31 25 pm

Any clue what might be going on?

@chinesedfan
Copy link
Member Author

@machour Thanks for your catch. I removed all node modules and got the same error. It seems that this PR updated some babel packages and introduced this bug. Let me find out reasons.

@chinesedfan
Copy link
Member Author

@machour I searched around and found that $export is not a function was usually caused by circular dependencies. After this PR applied, 2 different versions of core-js were installed.

It is an annoy feature of yarn that yarn add <pkg> only updates dependencies of pkg, but doesn't update those already in yarn.lock. Finally, I fixed it by keeping only one version of core-js.(In fact, if we just installed core-js 2.5.1, there were still no errors)

Copy link
Member

@machour machour left a comment

Choose a reason for hiding this comment

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

Seems to be working fine now, thank you!
Had a little test:
screen shot 2017-12-04 at 4 03 53 pm

I was expecting it to catch the bad values for position and left, but anyways, that's always a good additional tool to safeguard us :)

@coveralls
Copy link

coveralls commented Dec 4, 2017

Coverage Status

Coverage remained the same at 39.937% when pulling 662f043 on chinesedfan:add_stylelint into 2bc73fe on gitpoint:master.

@chinesedfan
Copy link
Member Author

@machour Hmm, seems like stylelint doesn't check each value. But if you like, there are a lot of rules. For example, declaration-property-value-whitelist can limit values. We can upgrade our configurations as we wish. 😄

@machour machour merged commit 7aaa5f6 into gitpoint:master Dec 5, 2017
@chinesedfan chinesedfan deleted the add_stylelint branch July 12, 2019 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants