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

fix: eslint warnings #1877

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

babygoat
Copy link
Collaborator

@babygoat babygoat commented Jun 3, 2021

This patch fixes several eslint warnings and adds the generated
sw.js to ignore error.

This patch fixes several eslint warnings and adds the generated
sw.js to ignore error.
@babygoat babygoat requested a review from taylrj June 3, 2021 07:25
@babygoat babygoat self-assigned this Jun 3, 2021
@babygoat
Copy link
Collaborator Author

babygoat commented Jun 3, 2021

@taylrj Only react/no-find-dom-node warnings are not resolved. I'm curious why u prefer to enforce the rule explicitly. Do u have any plans to migrate the API to react refs?

@taylrj
Copy link
Contributor

taylrj commented Jun 3, 2021

LGTM.

Yes, there's a plan to migrate the API since findDOMNode is going to be deprecated.
The alternative API usage needs some investigation, we don't have to include the change in this PR.

@babygoat
Copy link
Collaborator Author

babygoat commented Jun 3, 2021

The reason why I do the eslint fix is that I would like to enable the linter on CI but the warnings will break it. AFAIK, we need to put the refs on the child elements and use it for offset and height calculation, right?

@taylrj
Copy link
Contributor

taylrj commented Jun 7, 2021

AFAIK, we need to put the refs on the child elements and use it for offset and height calculation, right?

In theory, yes.

But I haven't figure out the way doing so and keep current behavior at the same time.
It might need more time to investigate how to refactor away use of findDOMNode.

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