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

fix(sc): Fix by stylelint #612

Merged
merged 5 commits into from
Nov 4, 2017
Merged

Conversation

chinesedfan
Copy link
Member

@chinesedfan chinesedfan commented Oct 31, 2017

WIP.

Modified by stylelint's suggestions. Including:

  • add missing commas
  • fix unexpected token
  • fix unsupported properties, like padding-horizonal
  • fix unitless shorthands properties
  • replace multi properties by shorthands

The last thing is that, is there a better way for empty tagged template? stylelint will throw a warning. Of course, maybe we can add a rule to ignore it.

const StyledListItem = styled(ListItem).attrs({
  ...
  hideChevron: props => props.unknown,
})``;

@coveralls
Copy link

Coverage Status

Coverage remained the same at 39.359% when pulling 8fb4aec on chinesedfan:fix_sc into 464aa53 on gitpoint:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 39.359% when pulling e00addb on chinesedfan:fix_sc into 464aa53 on gitpoint:master.

@@ -25,7 +25,7 @@ const StyledListItem = styled(ListItem).attrs({
},
underlayColor: props => (props.unknown ? null : colors.greyLight),
hideChevron: props => props.unknown,
});
})``;
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

@andrewda It is just my last commit, whose purpose is reverting changes. I removed them by mistake, because stylelint complained about empty blocks, as the updated PR description mentioned. But we can't simply do that. "fn``" is different with "fn".

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok yea, didn't see that that was a revert commit.

@@ -40,7 +40,7 @@ const CloseIcon = styled(Icon).attrs({
size: 28,
name: 'x',
type: 'octicon',
});
})``;
Copy link
Member

Choose a reason for hiding this comment

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

?

@chinesedfan chinesedfan merged commit e5c0291 into gitpoint:master Nov 4, 2017
@chinesedfan chinesedfan deleted the fix_sc branch November 4, 2017 03:14
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.

4 participants