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: [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event #24

Closed
wants to merge 9 commits into from
Closed

Fix: [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event #24

wants to merge 9 commits into from

Conversation

tienifr
Copy link

@tienifr tienifr commented Sep 14, 2023

Details

Fixed Issues

$ Expensify/App#26990
PROPOSAL: Expensify/App#26990 (comment)

Tests

  1. Open Expensify ND
  2. Verify that the warning: [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event does not appear in the JS console
  • Verify that no errors appear in the JS console

Screenshots/Videos

Web
wheel-compressed.mov

@tienifr
Copy link
Author

tienifr commented Sep 14, 2023

@thesahindia This one is ready for review!

@@ -0,0 +1,22 @@
/**
* Allows us to identify whether the browser supports passive event listener.
* Because older browsers will interpret any object in the 3rd argument of an event listener as capture=true.
Copy link
Member

Choose a reason for hiding this comment

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

NAB: Don't think this line is necessary

Suggested change
* Because older browsers will interpret any object in the 3rd argument of an event listener as capture=true.

Copy link
Author

Choose a reason for hiding this comment

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

Updated!

@tienifr
Copy link
Author

tienifr commented Sep 19, 2023

@thesahindia bump.

@thesahindia
Copy link
Member

@thesahindia bump.

@tienifr, can you help me with the steps that I need to get the warning? I am not getting it. Was there any specific step?

@getusha
Copy link

getusha commented Sep 19, 2023

@thesahindia @tienifr can we raise the PR upstream and apply the patch? it will be helpful since this fork is likely to be removed.

@tienifr
Copy link
Author

tienifr commented Sep 19, 2023

@getusha Thanks a lot! Can you give us the context on the removal of this fork?

@thesahindia What's your opinion?

@getusha
Copy link

getusha commented Sep 19, 2023

@tienifr we will be using react-native-web directly instead of this fork Expensify/App#24482

@thesahindia
Copy link
Member

@thesahindia @tienifr can we raise the PR upstream and apply the patch? it will be helpful since this fork is likely to be removed.

That makes sense to me!

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.

3 participants