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

Add prettier #182

Merged
merged 6 commits into from
Sep 2, 2017
Merged

Add prettier #182

merged 6 commits into from
Sep 2, 2017

Conversation

tobilen
Copy link
Collaborator

@tobilen tobilen commented Sep 2, 2017

adresses #181

  • Add prettier
  • Add eslint plugin to lint for prettier rules
  • Add npm script to autofix styling (yarn eslint:fix / npm run eslint:fix)
  • Extend linting to include stories folder
  • Apply styling to all existing files
  • Refactor .eslintrc to JavaScript file. This way, we can lint it, too.
  • Add section to readme about linting

The biggest part is the refactoring of the github issue and legacy stories. I was a bit unsure about refactoring the legacy stories. So maybe take an extra good look at those.

@Hypnosphi Hypnosphi self-requested a review September 2, 2017 01:43
@Hypnosphi
Copy link
Collaborator

Great work @tobilen! I'll take a look at the stories

@Hypnosphi
Copy link
Collaborator

Hypnosphi commented Sep 2, 2017

How do you feel about adding lint-staged and husky to run eslint --fix for staged files automatically on precommit hook?

@tobilen
Copy link
Collaborator Author

tobilen commented Sep 2, 2017

that'd basically mean modifying source files as a side effect. not sure thats a good idea

@Hypnosphi
Copy link
Collaborator

Hypnosphi commented Sep 2, 2017

Only those that are already modified in the commit (that's the point of lint-staged)

@tobilen
Copy link
Collaborator Author

tobilen commented Sep 2, 2017

im not a big fan of git hooks and husky in general. their main purpose is covered by well set up CI systems, and the automation part can be unexpected and unintuitive.

from a more practical standpoint, it apparently breaks a lot of GUI interfaces (typicode/husky#131, typicode/husky#125, lint-staged/lint-staged#151, ghooks-org/ghooks#40) or requires a lot of configuration (ghooks-org/ghooks#18) as well

@Hypnosphi
Copy link
Collaborator

Legacy stories work as expected. Please resolve conflicts and update the docs

… into feature/181

# Conflicts:
#	src/FlipMove.js
#	src/dom-manipulation.js
#	src/prop-converter.js
… into feature/181

# Conflicts:
#	src/FlipMove.js
#	src/dom-manipulation.js
#	src/prop-converter.js
Copy link
Collaborator

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

LGTM! Feel free to merge as soon as CI passes

@Hypnosphi
Copy link
Collaborator

By the way, you can now create branches directly in joshwcomeau/react-flip-move, this makes it a bit easier for other contributors to checkout =)

@tobilen
Copy link
Collaborator Author

tobilen commented Sep 2, 2017

alright, thanks. cant merge myself though, no write access.

@Hypnosphi Hypnosphi merged commit ca6e2ae into joshwcomeau:master Sep 2, 2017
@Hypnosphi
Copy link
Collaborator

Oh, looks like you haven't accepted the invite, please check your email

@joshwcomeau
Copy link
Owner

Woohoo! Awesome work :D

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