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(style): Make some common patterns more consistent #614

Merged
merged 1 commit into from
Oct 4, 2016

Conversation

jeffcarbs
Copy link
Member

@jeffcarbs jeffcarbs commented Oct 4, 2016

There were some things that we did in a lot of files but slightly differently. These aren't really lintable so there's no way to enforce them moving forward, but figured I take a pass through so at least it's more likely someone will copy/paste the "right" way 😄 . I tried to grep for some of this stuff when possible and chose the most common variation of the pattern:

  • Added className prop to MessageItem
  • Swapped order of className={classes} {...rest} to {...rest} className={classes} (the latter was way more common and makes more sense).
  • Use ElementType in a few places we weren't.
  • Always import 'classnames' as 'cx'
  • Always order setting variables: props, classes, rest, ElementType
  • Newline between setting variables and returning <ElementType>
  • When returning early because children, render a one-liner
  • Order children before className in props

@codecov-io
Copy link

codecov-io commented Oct 4, 2016

Current coverage is 100% (diff: 100%)

Merging #614 into master will not change coverage

@@           master   #614   diff @@
====================================
  Files         119    119          
  Lines        1915   1913     -2   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
- Hits         1915   1913     -2   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update 0b3db00...0adcc47

@@ -10,10 +8,7 @@ import Image from '../../elements/Image'
* An item can contain an image
* */
Copy link
Member

Choose a reason for hiding this comment

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

While we're at it, there are a number of doc blocks with * */ as a closing. This should just be */. The copy pasta there has been killing me a little :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed 👍

@levithomason
Copy link
Member

Also, there is technically a style(...): msg commit type we could use for this PR.

@levithomason
Copy link
Member

Github is back, the outage killed tests. I just retried them, we'll see.

- Added className prop to MessageItem
- Swapped order of `className={classes} {...rest}` to `{...rest} className={classes}`
- Use ElementType in a few places we weren't
- Always import 'classnames' as 'cx'
- Always order setting variables: props, classes, rest, ElementType
- When returning early because children, render a one-liner
- Order children before className in props
- Fix closing docbock "* */" => "**/"
@jeffcarbs
Copy link
Member Author

Was missing an import. Fixed and pushed up.

@levithomason
Copy link
Member

Ready to go here then?

@levithomason
Copy link
Member

Going!

@levithomason levithomason merged commit fcb61e4 into master Oct 4, 2016
@levithomason levithomason deleted the chore/cleanup branch October 4, 2016 07:41
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.

3 participants