-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Hide devtools when closed, prevent focus #2376
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/tannerlinsley/react-query/EMHjfN3HuJqW2qhZrXdmEUCt2MJe |
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 4386d67:
|
Oh cool, you can see it working in the codesandbox as well. I haven't seen a github codesandbox bot before, that's pretty nifty! |
One of the best bots :) |
Thank you 👍 . I've tested this in the codesandbox, and it looks good. While testing, I found a semi-related issue that I probably introduced when I made the devtools unsubscribe when they are hidden:
I think this happens because we put the queries into local state: and then update in the effect when we are subscribed: but since the update happens when we are closed, the component only re-renders when we open it and thus it still has the old queries in the local state. one way to fix it is to really unmount when we close and mount again, because it would pick up the initial state of I understand if you don't want to tackle this at all, and we can open a separate issue as well. But if we decide to go for the unmount/remount solution because of this finding, I would rather not add the |
If we unmount/mount, we would lose other local state like filtering and such. Is that acceptable? If we want to keep that state, it sounds like your suggested approach would be to:
Are there any downsides to that approach? It sounds relatively straightforward, and I can try it out here if you'd like. P.S. Good catch! |
cool, go ahead 😊 |
I believe I've fixed the stale queries, but just realized I've introduced a regression. Opening the devtools and refreshing the page results in a |
Alright, I think I've taken care of that too. Should be ready for another set of 👀 . Thanks! |
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.
great work, thanks 🚀
🎉 This PR is included in version 3.17.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #2375.
Previously, opening and closing the devtools only changed it opacity (and subscribed/unsubscribed to queries). That meant that it was still possible to tab through the elements in the panel, even when it was closed.
With this change, when the panel starts out closed, we set a
visibility: hidden
style, so that it is gone from both the accessibility tree and the tab order. I've also set up transition listeners so that before a transition starts, if the devtools are opening, we turn visibility to visible, so that the opacity fade-in works correctly. Likewise, on a transition end, we check if the panel is closed, and turn visibility to hidden.I tested this by compiling, copying the built files into my project's node_modules, and running my app, and it worked as I expected. The fade still occurs (both in and out), focus does not move into a closed devtools, and the devtools are hidden in the accessibility tree.
This probably would have been more straightforward with a keyframe animation, but I am not sure how to do that using inline styles, or whether its even possible.