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 #2252

Merged
merged 2 commits into from
Dec 23, 2021
Merged

Fix ESLint warnings #2252

merged 2 commits into from
Dec 23, 2021

Conversation

asumaran
Copy link
Contributor

@asumaran asumaran commented Dec 22, 2021

Fixes a few ESLint warnings that were being displayed by GitHub on PRs - Here's one example.

There's no refence element to get the window object that's why the rule is being disabled instead.
@asumaran asumaran self-assigned this Dec 22, 2021
@asumaran asumaran marked this pull request as ready for review December 22, 2021 23:38
@@ -28,9 +28,12 @@ const useConfirmNavigation = ( displayPrompt ) => {
event.preventDefault();
event.returnValue = '';
};

// eslint-disable-next-line @wordpress/no-global-event-listener
Copy link
Contributor Author

@asumaran asumaran Dec 22, 2021

Choose a reason for hiding this comment

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

I'm disabling the rule here in the next line as there's no element that we can use to get the defaultView object.

Copy link
Member

@ricardo ricardo left a comment

Choose a reason for hiding this comment

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

As some people have asked in the PR where the rule was introduced, I'm also not familiar as to why this warning is important. It looks a bit overkill to me - not judging the specific change, but the need for it 🤔. What do you think @asumaran and @harriswong?

Perhaps not even worth discussing as this only applies for 2 files 😅.

The change LGTM 👍.

@asumaran asumaran merged commit a86630b into develop Dec 23, 2021
@asumaran asumaran deleted the as-fix-eslint branch December 23, 2021 05:41
@harriswong
Copy link
Contributor

Hi @asumaran @ricardo , sorry for the late reply! I am good with the // eslint-disable-next-line as well. 👍

I think the idea was to prevent the use of listener on window document globals inside hooks and react components. It looks like the no global rule is removed in WordPress/gutenberg#34528. Maybe we can try updating our @woocommerce/eslint-plugin to the latest 1.3?

@asumaran
Copy link
Contributor Author

asumaran commented Jan 7, 2022

@harriswong Thanks for the recommendation. I've created an issue #2270 to update our @woocommerce/eslint-plugin package.

@asumaran asumaran mentioned this pull request Jan 11, 2024
4 tasks
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.

None yet

3 participants