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

[FacilityLocator] Prettier Linting Changes #9071

Merged
merged 5 commits into from
Nov 16, 2018
Merged

Conversation

r-mc2
Copy link
Contributor

@r-mc2 r-mc2 commented Nov 7, 2018

Description

Follow up to previous FacilityLocator PR to do the 2nd pass for Prettier's syntax needs.

  • Removed the global disables from all files.
  • Left a few single-line disables as the "prettier" version was more unreadable.
  • Quick fix to API to support that Provider records also come back wrapped in a 'data' block
  • Restructured some Action logic to use async/await syntax
  • Other small changes as suggested in previous PR ([FacilityLocator] Provider Locator Features (& Fixes) #8651)

Acceptance criteria

  • Prettier rules re-enabled across all of the Locator with small one-liner ignores

(CC'ing reviewers for visibility @U-DON @bshyong )

* Removed the global disables from all files.
* Left a few single-line disables as the "prettier" version was more unreadable.
* Quick fix to API to support that Provider records also come back wrapped in a 'data' block
* Restructured some Action logic to use async/await syntax
* Other small changes as suggested in previous PR (#8651)
@r-mc2 r-mc2 requested review from bshyong and U-DON November 7, 2018 21:33
@va-bot va-bot temporarily deployed to vetsgov-pr-9071 November 7, 2018 21:34 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-9071 November 7, 2018 21:39 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-9071 November 7, 2018 21:56 Inactive
@va-bot va-bot temporarily deployed to master/provloc-mvp-cleanup November 7, 2018 22:11 Inactive
Copy link
Contributor

@U-DON U-DON left a comment

Choose a reason for hiding this comment

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

LGTM, but it seems like a lot of disables were added to keep the parens around single argument arrow functions. I don't think these should be necessary, and if we want to change the Prettier rules, we should have a larger discussion with the FE devs.

* @param {Object} query The current state of the Search form
*/
// eslint-disable-next-line prettier/prettier
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these still disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarity. Not having the method's argument list inside parens makes it harder to see when using arrow function syntax. Using arrow functions shouldn't need to remove the basics of function arguments/parameters being inside parenthesis.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that it still requires parens when there are more than one argument.

@@ -1,4 +1,3 @@
/* eslint-disable prettier/prettier */
/* eslint-disable no-use-before-define */
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should need to be disabled. Should be fixed if it's easy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is the linter not liking the order of declarations. renderPhoneNumber is a support function and below the SFC LocationPhoneLink, however, it is used a few times inside the main body of the SFC.
Fixing it would require the helper be moved up to the top of the file which would be confusing as this is the LocationPhoneLink component so that should be first.

It doesn't actually break the code or functionality of the component so I disabled it. (Near as I can tell this rule is only checking where in the file the function was declared and where it was used (i.e. line#). But no actual checks on whether or not it would actually cause a problem.)

@va-bot va-bot temporarily deployed to vetsgov-pr-9071 November 15, 2018 18:54 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-9071 November 15, 2018 22:12 Inactive
@va-bot va-bot temporarily deployed to master/provloc-mvp-cleanup November 15, 2018 22:27 Inactive
@r-mc2 r-mc2 merged commit b49ebba into master Nov 16, 2018
ATeal added a commit that referenced this pull request Nov 19, 2018
* master:
  526 empty supporting evidence mg (#9106)
  Move assets into /src/site/assets (#9138)
  Create src/site/stages (#9130)
  Move most of va-gov's contents to src/site (#9127)
  Remove the submit promise (#9140)
  turn on online only and distance learning (#9090)
  Bump formation version to fix headings (#9141)
  Add POW to specialIssues array (#9131)
  GIBS wizard (#9107)
  [FacilityLocator] Prettier Linting Changes (#9071)
@ncksllvn ncksllvn deleted the provloc-mvp-cleanup branch November 28, 2018 18:19
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