Remove unneeded/incorrect eslint settings and add (back?) jsx-a11y #280
Conversation
| 'react/jsx-pascal-case': 2, | ||
| 'react/sort-prop-types': [0, {'callbacksLast': true, 'requiredFirst': true}], // TODO(awong): Too hard to turn on. | ||
| 'react/jsx-sort-props': [0, {'callbacksLast': true, 'shorthandFirst': true}], // TODO(awong): Too hard to turn on. | ||
| 'react/jsx-no-duplicate-props': 2, | ||
| 'react/no-danger': 2, | ||
| 'react/no-deprecated': 2, | ||
| 'react/no-direct-mutation-state': 2, | ||
| 'react/no-string-refs': 0, // TODO(awong): Enable. |
There was a problem hiding this comment.
There are a bunch of things that have been deleted here and I'm wondering why? Some of them are things I enabled in the starter app, including no-string-refs and jsx-indent-props.
There was a problem hiding this comment.
The 0 means disable, so deleting this line should turn it on? Or does it need to be explicitly set to 2? I may have erroneously assumed they were disabling it since they did it explicitly here.
There was a problem hiding this comment.
Oh, good question. I guess I assumed it needs to be explicitly set to 2 because many of the other rules are configured that way, but I could be wrong! Reading the docs it's not totally clear, although this page says that "No rules are enabled by default", so perhaps it does need to be explicitly set with 2?
There was a problem hiding this comment.
The presets like airbnb turn on a lot of stuff so I assumed it was being explicitly turned off here to override that. There are comments to that effect. I can try turning it on explicitly to see if we start getting errors but would like to avoid leaving that in if the default is already okay.
There was a problem hiding this comment.
Ah okay, I see that now. Sounds good.
There was a problem hiding this comment.
@annekainicUSDS I checked these properties and they are enabled by default in the Airbnb setup. Anything else need to be done here?
| @@ -97,7 +97,7 @@ export default function SubmitButtons(props) { | |||
| if (process.env.NODE_ENV !== 'production') { | |||
| submitButton = ( | |||
| <div className="small-6 usa-width-one-half medium-6 columns"> | |||
| <a onClick={onSubmit}>Submit again</a> | |||
| <button onClick={onSubmit}>Submit again</button> | |||
There was a problem hiding this comment.
I know the Vets.gov team made this a link for a specific reason, and not that we should be beholden to that, but I think this kind of change might need more review. Would be curious for their input on this.
There was a problem hiding this comment.
It doesn't apply in production so web site users should never see it? Or is the vets.gov site run in development mode? From an a11y point it really is a button and not a link (do something vs go somewhere) so there's that.
Looking at the rest of the file which still has references to the vets.gov site, I figured this was a pretty low-risk change. At the moment it's not possible to submit a form using USFS unless you have a server that works just like the vets.gov one, so this code won't be reached except by vets.gov.
There was a problem hiding this comment.
Yes, I think it's only a feature used for development and not something real users should ever see. I remember some discussion around this feature awhile back, though I can't remember the reasoning for making it a link, so I just wanted to double check with them in case I'm missing anything.
There was a problem hiding this comment.
🤔 I don't remember why we went with an anchor instead of a button. Seems pretty safe to me, though.
There was a problem hiding this comment.
I think we made this a link for style reasons. It should really be a button. Like Dave says, this is never shown in production, so it's a low-risk change.
Also, this extra button isn't super useful to us anymore, because we used to use it in our staging environment, but that's run in production mode. The conditional used to be for our buildtype flag instead of NODE_ENV, which doesn't make sense in a separate library.
There was a problem hiding this comment.
If so let's add that as a task to #211. I only made minimal changes in this PR to get eslint to pass. I suppose I could have done that with a disable-next-line comment as well but I'd have to look at whether you can do that inside a JSX block.
There was a problem hiding this comment.
Most of our forms are wrapped in our save-in-progress functionality, so submitting the form again should be pretty easy as far as the vets.gov team is concerned--just load the saved form and try again.
I personally haven't used that retry button link in a long time. It's probably safe to remove.
In reviewing some of the recent a11y tickets I noticed that our eslint setup was referencing
jsx-a11yrules but we didn't have that plugin listed. I've done that but also did a quick pass through to re-enable any rules that were disabled but don't give errors on the current setup--maybe they did so before it was extracted from vets.gov?That said, I still see several eslint plugins in
package.jsonthat don't seem to be in.eslintrcso there is at least one more pass to make here.Types of changes
Checklist:
npm run lint.npm run build.I added tests to verify a bug fix or new functionality.(Tests updated to pass lint changes).npm test.