-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
♻️ Refactor Focus Manager Event Handling for Enhanced User Experience ✨ #2672
base: main
Are you sure you want to change the base?
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 17d1760:
|
Like it, although there're some tests that we need to update. |
Ok, sure! But could you please guide me through it? I apologize for any inconvenience caused. |
Most of tests are based on |
Okay :) |
I apologize for the inconvenience, but I'm having trouble updating the test code correctly. I'm not familiar with the process, and I don't want to risk introducing errors or breaking anything. Could someone please lend me a hand and guide me through the correct steps? I'm sorry for any delays this may cause, and I truly appreciate your assistance. |
https://github.com/TanStack/query/pull/4805/files#diff-3cd8cae55f836a38ea976d6cbf9f1b59e8fdfa4d3218b123cb1f9dccfc6824d2 |
Changes look good to me but this might need to be landed in v3 as it's breaking. |
Okay 🙌 And thanks a lot @promer94 |
After this change has landed, we should change the example ( I feel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
Passed | Secrets | 0 0 0 0 | View in Orca |
Any updates on how close is the v3? Can we have like future flags or a canary channel? |
As you can see, the focus manager currently listens to both
visibilitychange
andfocus
events by default.I believe it may be beneficial to consider discontinuing the listening to
focus
events and solely rely onvisibilitychange
events.My rationale behind this proposal is that we might be supporting both events primarily for historical reasons. It is worth noting that
visibilitychange
was not fully supported in older browsers that are no longer relevant (e.g., IE11).The
focus
event has proven to have various pitfalls, as it can be triggered in the following scenarios:These scenarios have the potential to result in a suboptimal user experience.
This pull request is inspired by this reference PR.