-
Notifications
You must be signed in to change notification settings - Fork 275
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
[Website] Hide Settings menu after clicking "Restore from .zip #1904
Conversation
Nice! Thank you for contributing! One thing that stood out to me is the modal provider. The other buttons in that dropdown menu use the redux wordpress-playground/packages/playground/website/src/components/layout/index.tsx Lines 163 to 169 in 8484cce
|
@adamziel Improvements are ready. |
That failing tests is an issue with GitHub CI setup, I just restarted it. |
The code change looks good to me, let me also loop in @brandonpayton for another review and merging. Thank you so much for contributing @ajotka ! |
I'll go ahead and merge, @brandonpayton if you'll notice something later on feel free to still share your feedback |
Thank you so much for your contribution @ajotka and congratulations on your first merged PR! 🎉 |
Thanks, Adam. I don't have any other feedback except that it is nice this PR moves the modal definition out of the button module. Thanks, @ajotka! |
Motivation for the change, related issues
Issue #1903
Implementation details
Because Modal was in the same component as Dropdown Menu it was always visible when Dropdown was open (and disappears when I tried to force close menu item, because of unmounting).
So I decided to wrap dropdown with context and move there whole logic.
Testing Instructions (or ideally a Blueprint)
Just open Playground and go to settigns.
Screen with results