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

Set up Prettier for automated front end code formatting #861

Merged
merged 9 commits into from
Jan 19, 2022
Merged

Conversation

seancolsen
Copy link
Contributor

Fixes #767

Before

<li role="presentation" class="tab" class:active={isActive} tabindex="-1" 
    style={isActive ? null : `width: ${Math.floor(100 / totalTabs)}%;`}>

  <a role="tab" href={getTabURL(tab) || '#'} tabindex="0"
      aria-selected={isActive} aria-disabled="{!!tab.disabled}"
      id={isActive ? `mtsr-${componentId}-tab` : null} data-tinro-ignore
      aria-controls={isActive ? `mtsr-${componentId}-tabpanel` : null}
      on:focus on:blur on:mousedown
      on:click>
        <slot></slot>
  </a>

  {#if allowRemoval}
    <button type="button" aria-label="remove" class="remove"
      on:click={(e) => dispatch('remove', e)}>
      &times;
    </button>
  {/if}
</li>

After

<li
  role="presentation"
  class="tab"
  class:active={isActive}
  tabindex="-1"
  style={isActive ? null : `width: ${Math.floor(100 / totalTabs)}%;`}
>
  <a
    role="tab"
    href={getTabURL(tab) || '#'}
    tabindex="0"
    aria-selected={isActive}
    aria-disabled={!!tab.disabled}
    id={isActive ? `mtsr-${componentId}-tab` : null}
    data-tinro-ignore
    aria-controls={isActive ? `mtsr-${componentId}-tabpanel` : null}
    on:focus
    on:blur
    on:mousedown
    on:click
  >
    <slot />
  </a>

  {#if allowRemoval}
    <button
      type="button"
      aria-label="remove"
      class="remove"
      on:click={(e) => dispatch('remove', e)}
    >
      &times;
    </button>
  {/if}
</li>

Notes

  • My approach to getting prettier and eslint to play nicely together was as follows:
    1. Format with prettier.
    2. Keep eslint, but turn off all the rules that were failing as a result of the formatting.
  • You can see which rules I turned off in 34d9a52
  • The only non-formatting rules that I turned off are @typescript-eslint/no-unsafe-assignment and @typescript-eslint/no-unsafe-call
    • We already agreed to turn those off in .svelte files.
    • I kept them on in .ts files.
  • I wouldn't be surprised if more eslint failures pop up that we need to turn off. For our team, I'm not so worried about that, but it could be frustrating for external contributor who don't understand wha't going on during this period of flux.

Tips on resolving merge conflicts from this PR

Since this PR touches basically every file, it'll surely result in merge conflicts -- but they shouldn't be hard to resolve.

  • If this PR gets merged first and you're trying resolve conflicts in another PR, accept the the changes in your other PR, then run npm run format and commit.
  • If this PR goes stale and you want to replay it from scratch with minimal conflicts, you can cherry-pick all commits except "Format with prettier". Then run npm run format and commit the changes.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@pavish
Copy link
Member

pavish commented Dec 2, 2021

@seancolsen I'd like to merge this PR after all the current open PRs are merged, to avoid fixing merge conflicts across all of them.

@pavish
Copy link
Member

pavish commented Jan 14, 2022

@seancolsen Could you please update this PR to sync changes from master?

I'd still like to merge this only after all the current open frontend PRs (at the time of time of this comment) are merged.

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2022

Codecov Report

Merging #861 (17e9edc) into master (8d92794) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #861   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files          86       86           
  Lines        3161     3161           
=======================================
  Hits         2949     2949           
  Misses        212      212           
Flag Coverage Δ
pytest-backend 93.29% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d92794...17e9edc. Read the comment docs.

@seancolsen seancolsen changed the base branch from master to 619_ts_strict January 14, 2022 20:14
Base automatically changed from 619_ts_strict to master January 18, 2022 22:44
Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

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

@seancolsen Looks good to me!

@pavish pavish enabled auto-merge January 19, 2022 21:31
@pavish pavish merged commit eef6175 into master Jan 19, 2022
@pavish pavish deleted the 767_prettier branch January 19, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Improve frontend code linting/formatting
3 participants