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

Eliminate no-global-event-listener ESLint warnings #5732

Merged
merged 4 commits into from
Dec 23, 2020

Conversation

johnwatkins0
Copy link
Contributor

@johnwatkins0 johnwatkins0 commented Dec 22, 2020

Summary

Fixes #5731

This eliminates the @wordpress/no-global-event-listener ESLint warnings shown in #5731. A few different approaches were used:

  1. In non-React files, where the rule is less relevant, used eslint-disable-next-line to the three lines that had the issue global is used instead of window.
  2. For the UnsavedChangesWarning component, passed in the app root and used the root's ownerDocument, because doing so was simple.
  3. For the useWindowWidth hook, replaced window with global because that hook is intended to use the global window.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@johnwatkins0 johnwatkins0 self-assigned this Dec 22, 2020
@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #5732 (b406119) into develop (5ce0a95) will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #5732   +/-   ##
==========================================
  Coverage      73.66%   73.66%           
  Complexity      5429     5429           
==========================================
  Files            186      186           
  Lines          16375    16375           
==========================================
  Hits           12063    12063           
  Misses          4312     4312           
Flag Coverage Δ Complexity Δ
javascript 66.81% <50.00%> (ø) 0.00 <0.00> (ø)
php 73.86% <ø> (ø) 0.00 <ø> (ø)

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

Impacted Files Coverage Δ Complexity Δ
assets/src/utils/use-window-width.js 80.00% <50.00%> (ø) 0.00 <0.00> (ø)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2020

Plugin builds for b406119 are ready 🛎️!

@pierlon pierlon added the WS:UX Work stream for UX/Front-end label Dec 22, 2020
@pierlon pierlon added this to the v2.0.10 milestone Dec 22, 2020
@@ -46,10 +46,12 @@ const addBeforeUnloadPrompt = () => {
if ( beforeUnloadPromptAdded ) {
return;
}
// eslint-disable-next-line @wordpress/no-global-event-listener
window.addEventListener( 'beforeunload', onBeforeUnload );
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use global here since the file is being transpiled via Webpack, which would then provide reference to window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. Might as well use global in all these instances. Updated in 747061f

window.addEventListener( 'beforeunload', onBeforeUnload );

// Remove prompt when clicking trash or update.
document.querySelector( '#major-publishing-actions' ).addEventListener( 'click', () => {
// eslint-disable-next-line @wordpress/no-global-event-listener
window.removeEventListener( 'beforeunload', onBeforeUnload );
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 747061f

@@ -144,6 +144,7 @@ class ErrorRows {
} );
};

// eslint-disable-next-line @wordpress/no-global-event-listener
window.addEventListener( 'click', ( event ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 747061f

@johnwatkins0 johnwatkins0 requested a review from pierlon December 22, 2020 22:06
Co-authored-by: Weston Ruter <westonruter@google.com>
@westonruter westonruter merged commit 2dc8341 into develop Dec 23, 2020
@westonruter westonruter deleted the fix/5731-global-event-listeners branch December 23, 2020 19:46
westonruter added a commit that referenced this pull request Dec 30, 2020
@westonruter westonruter modified the milestones: v2.0.10, v2.1 Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WS:UX Work stream for UX/Front-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix warnings from @wordpress/no-global-event-listener ESLint rule
3 participants