Add prettier and eslint, .npmrc #28

shawnbot · 2018-05-21T20:52:57Z

This adds prettier and eslint with the github config, and runs the lint script on Travis.

~~This fails on Travis as of 5/21 and #21 being merged, so I'm going to see if I can fix it.~~ Fixed!

shawnbot · 2018-05-21T22:35:55Z

Well, this is super frustrating. Without a jsx-quotes rule, running npm run lint -- --fix (i.e. eslint src --fix) fixes all of the linting errors but changes all of the JSX attribute quotes to double. But if I add "jsx-quotes": ["warn", "prefer-single"] to the eslint config and then run eslint --fix, it treats all of the single quotes as errors and fails.

I'm going to close this for now, but I'd like to get linting working soon.

shawnbot · 2018-05-21T22:49:03Z

If/when we decide to pick this up again, it appears that the issue is with Prettier:

I'm not sure what the solution is, other than to wrap all of the attributes in curly braces. 😠

emplums · 2018-06-08T19:14:27Z

@shawnbot how do you feel about merging this in as is? JSX prefers double quotes for attributes since that's similar to the HTML api for attributes, and several major linters & lint configs use this convention ex. airbnb's eslint config here. It is a little weird to switch between single quotes for JS and double quotes for attributes (which are also technically js lol), but I don't think it should hold us back from getting linting & formatting set up for all the other cases we want to lint/format. Plus, if you want to keep writing single quotes for attributes you can, and we can just run the eslint fix in CI 🤷‍♀️

I did see that there's a fork of prettier that we might be able to use with a a jsx-single-quotes rule but it's implementation seems a bit tricky https://github.com/arijs/prettier-miscellaneous

shawnbot · 2018-06-08T22:19:59Z

I’m down with that! JSX quotes should probably be a warning so that they don’t fail builds, right?

shawnbot

Might need to undo some of the ~~limiting~~ linting fixes to avoid conflicts.

shawnbot · 2018-06-08T22:21:08Z

package.json

@@ -18,6 +18,7 @@
  "license": "MIT",
  "main": "index.js",
  "scripts": {
+    "lint": "eslint src",


Also, we should lint examples.

shawnbot · 2018-06-21T18:24:41Z

@emplums I got this working and eslint --fix'd all of the things. As of 828d50c there are a ton of warnings, mostly related to prop-types) but zero errors!

emplums · 2018-06-21T18:36:00Z

Oh awesome! Let's just go with this then instead of standard 🎉 Would you mind adding some documentation about the linter to the README as part of this?

shawnbot · 2018-06-21T20:48:28Z

@emplums How's this section look to you?

shawnbot · 2018-06-25T21:07:43Z

.eslintrc.json

+    "plugin:github/react"
+  ],
+  "rules": {
+    "jsx-a11y/no-autofocus": "warn",


I left a note in the commit about this, but I wanted to flag it for @emplums because it has a11y implications. We'll need to remove support for autofocus from <TextInput> if we remove this line, which will fail any instance of it.

Hmm... I think we might want to remove support for autofocus on TextInputs anyways, unless there's a strong case for it. It can have pretty bad a11y consequences. I'll open a separate issue for that . 👍

Created an issue for discussion here: #89

shawnbot · 2018-06-25T21:08:30Z

.eslintrc.json

+  "rules": {
+    "jsx-a11y/no-autofocus": "warn",
+    "react/prop-types": "warn",
+    "react/sort-prop-types": "warn"


Once we fix up prop types sorting we should remove this. It just didn't seem to be an error to me if prop type keys aren't sorted alphabetically.

shawnbot · 2018-06-25T21:09:31Z

src/Text.js

+        className,
+        color && `text-${color}`,
+        fontSizeClass,
+        fontWeight === 'bold' && 'text-bold',


Linting uncovered the fact that we weren't actually using fontWeight anywhere, so I added support for fontWeight='bold' here. But maybe it should just be weight or a boolean prop bold?

I like fontWeight since it follows the primer/primer api!

shawnbot · 2018-06-25T21:11:41Z

src/Dropdown.js

+  <div className="BtnGroup">
+    <Details className="details-reset BtnGroup-form d-flex">
+      {(
+        {open, toggle} // eslint-disable-line no-unused-vars


In retrospect, it looks like we can just nix open from this destructure and kill the eslint-disable-line comment.

shawnbot · 2018-06-25T21:12:42Z

@emplums It's green and I left a couple of notes. Do you mind doing one more pass over this before we merge? 🙇

shawnbot · 2018-06-26T16:16:29Z

We have a ton of jsx-a11y rules! The ones in our config are just overrides so that certain ones don't fail our builds.

emplums

Just left a few comments and questions! The only thing pending that I think we should include before merge is the full set of jsx-a11y rules. I'm also keen on adding a rule around spacing in objects, as it helps make the code more readable, IMO.

emplums · 2018-06-26T16:34:06Z

src/Avatar.js

-    src,
-    ...rest
-  } = props
+  const {alt, isChild, size = 20, src} = props


Can we add the object-curly-spacing eslint rule for consistent spacing within objects 😬

My personal preference is not to, and I feel like this is the type of deviation from the GitHub config that might make it weird for folks outside our team who want to contribute. How do we make a decision like this? ~~Put it to a vote?~~ 😬 /cc @broccolini

emplums · 2018-06-26T16:36:55Z

src/Text.js

+        className,
+        color && `text-${color}`,
+        fontSizeClass,
+        fontWeight === 'bold' && 'text-bold',


I like fontWeight since it follows the primer/primer api!

emplums · 2018-06-26T16:38:12Z

src/TextInput.js

-    size,
-    value
-  }) => (
+const TextInput = ({autocomplete, autofocus, block, disabled, id, name, placeholder, required, size, value}) => (


Does this rule get enforced no matter how many object keys there are? Or is it only enforced if the line is less than a certain number of characters?

I think it's a line length thing. 🙁

emplums · 2018-06-26T16:42:54Z

.eslintrc.json

+    "jsx-a11y/no-autofocus": "warn",
+    "react/prop-types": "warn",
+    "react/sort-prop-types": "warn"
+  }


Can we add the jsx-a11y/recommended plugin here so that we get the full set of a11y rules?

shawnbot · 2018-06-27T17:27:05Z

@emplums I added the jsx-a11y recommended rules! 🎉 You cool punting on the curly spacing rule for now, merging this as-is, and discussing any changes at our API meeting?

shawnbot added 4 commits May 21, 2018 13:35

save exact versions via npm

9a7cd17

add prettier + eslint configs

12763a0

add lint script

51da841

run lint and test on travis

d740d3f

shawnbot changed the title ~~Add prettier and eslint, .npmrc~~ [WIP] Add prettier and eslint, .npmrc May 21, 2018

shawnbot added 9 commits May 21, 2018 14:14

use key in <link> map() output

d4293d6

kill hanging comma

79d15a0

nix unused props in <Index>

c56f200

add jest plugin to eslint config

6ea9f2f

Merge remote-tracking branch 'origin/master' into add-prettier

8e7d31d

update package-lock.json

0e6542f

run the build script on Travis

85bfbaf

nix Fragment import

f960cd8

only do jest rules in src/__test__

68060de

shawnbot closed this May 21, 2018

shawnbot mentioned this pull request Jun 7, 2018

Dropdown #57

Merged

shawnbot reopened this Jun 8, 2018

shawnbot commented Jun 8, 2018

View reviewed changes

shawnbot deployed to github-pages June 21, 2018 18:22 View deployment

shawnbot deployed to github-pages June 21, 2018 20:47 View deployment

shawnbot deployed to github-pages June 21, 2018 20:49 View deployment

shawnbot requested a review from emplums June 21, 2018 20:54

shawnbot changed the title ~~[WIP] Add prettier and eslint, .npmrc~~ Add prettier and eslint, .npmrc Jun 21, 2018

shawnbot added 9 commits June 25, 2018 13:56

lint

54ce0de

lint (whitespace only)

a10d292

lint

ce1e9cb

lint

178c2f5

lint

84da4b8

lint

65c31b8

lint DonutChart

51b5551

lint DonutSlice

dab62dc

warn about jsx-a11y/no-autofocus /cc @emplums

916dfd4

shawnbot deployed to github-pages June 25, 2018 21:04 View deployment

shawnbot commented Jun 25, 2018

View reviewed changes

nix unused "open" render prop

41b2c4b

shawnbot deployed to github-pages June 25, 2018 21:14 View deployment

emplums mentioned this pull request Jun 26, 2018

0.0.6-beta release tracking #86

Merged

7 tasks

shawnbot mentioned this pull request Jun 26, 2018

Use jsx-a11y/recommended eslint config #88

Closed

emplums suggested changes Jun 26, 2018

View reviewed changes

add all jsx-a11y/recommended eslint rules

3cb6512

shawnbot deployed to github-pages June 27, 2018 17:26 View deployment

emplums approved these changes Jun 27, 2018

View reviewed changes

shawnbot merged commit ce23d82 into release-0.0.6-beta Jun 27, 2018

shawnbot deleted the add-prettier branch June 27, 2018 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prettier and eslint, .npmrc #28

Add prettier and eslint, .npmrc #28

shawnbot commented May 21, 2018 •

edited

Loading

shawnbot commented May 21, 2018

shawnbot commented May 21, 2018

emplums commented Jun 8, 2018 •

edited

Loading

shawnbot commented Jun 8, 2018

shawnbot left a comment •

edited

Loading

shawnbot Jun 8, 2018

shawnbot commented Jun 21, 2018

emplums commented Jun 21, 2018

shawnbot commented Jun 21, 2018 •

edited

Loading

shawnbot Jun 25, 2018

emplums Jun 26, 2018

emplums Jun 26, 2018

shawnbot Jun 25, 2018

shawnbot Jun 25, 2018

emplums Jun 26, 2018

shawnbot Jun 25, 2018

shawnbot commented Jun 25, 2018

shawnbot commented Jun 26, 2018

emplums left a comment

emplums Jun 26, 2018

shawnbot Jun 26, 2018 •

edited

Loading

emplums Jun 26, 2018

emplums Jun 26, 2018

shawnbot Jun 26, 2018

emplums Jun 26, 2018

shawnbot commented Jun 27, 2018

Add prettier and eslint, .npmrc #28

Add prettier and eslint, .npmrc #28

Conversation

shawnbot commented May 21, 2018 • edited Loading

shawnbot commented May 21, 2018

shawnbot commented May 21, 2018

emplums commented Jun 8, 2018 • edited Loading

shawnbot commented Jun 8, 2018

shawnbot left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbot commented Jun 21, 2018

emplums commented Jun 21, 2018

shawnbot commented Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbot commented Jun 25, 2018

shawnbot commented Jun 26, 2018

emplums left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbot Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbot commented Jun 27, 2018

shawnbot commented May 21, 2018 •

edited

Loading

emplums commented Jun 8, 2018 •

edited

Loading

shawnbot left a comment •

edited

Loading

shawnbot commented Jun 21, 2018 •

edited

Loading

shawnbot Jun 26, 2018 •

edited

Loading