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

chore: update to new eslint-config #75

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Conversation

satazor
Copy link
Collaborator

@satazor satazor commented Mar 19, 2020

This PR updates to the new eslint-config with a11y enabled as well. There's no warnings besides these ones:

Screenshot 2020-03-19 at 20 26 12

These are related a11y + Next.js Link API. Associated threads:

What should we do? Ignore warnings? Or implement a custom Link to deal with this issue?

@vercel
Copy link

vercel bot commented Mar 19, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/moxystudio/next-with-moxy/fary21qei
✅ Preview: https://next-with-moxy-git-chore-new-eslint-config.moxystudio.now.sh

@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #75 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #75   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines           47        47           
  Branches         7         7           
=========================================
  Hits            47        47           

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 5f39f0b...2aef320. Read the comment docs.

@satazor
Copy link
Collaborator Author

satazor commented Mar 19, 2020

@acostalima we might want to do the same for rnwm.

Could you please create a follow-up issue in the repository? Thanks!

@threequartersjohn
Copy link
Contributor

I think we might explicitly unset the rule in our eslintrc.json. I don't think this is a problem with the plugin, but Next.js's implementation. This also makes it easy to rollback if the Link component is changed to have a different API.

Even custom Link components will likely set href conditionally, which would also fire this warning. Also, in Next.js's Link component the href is a required prop, which already safeguards against the presence of an hrefattribute.

@satazor
Copy link
Collaborator Author

satazor commented Mar 19, 2020

@threequartersjohn let me elaborate on the custom link component: we could do a custom Link component in src/shared/components that would use a anchor by default and would also support a custom render prop in cases we want to render anything else. The warning would still be there, but we would ignore (with eslint-ignore) just in that file.

I also like disabling the rule. Let’s ask for feedback from @moxystudio/developers.

@satazor
Copy link
Collaborator Author

satazor commented Mar 23, 2020

Disabled the rule for now.

@satazor satazor merged commit 4e09b3b into master Mar 23, 2020
@satazor satazor deleted the chore/new-eslint-config branch March 23, 2020 00:57
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.

2 participants