Fix: Import & Export from Github causes reloading the playground even before accept this step. #1908
Conversation
packages/playground/website/src/github/github-export-form/modal.tsx
Outdated
Show resolved
Hide resolved
|
@adamziel ready to check |
|
@adamziel I have kept the functionality of opening the modal on the URL parameter, but I have also added a lock to not reload WordPress if only the 'modal' parameter changes in the URL parameters. And also, there is no signal() anymore. So you can use this functionality for other modals as well, or extend it with other searchParams keys. Ready to re-check. |
| const notRefreshingParam = 'modal'; | ||
| const oldParams = new URLSearchParams(prevUrl?.search); | ||
| const newParams = new URLSearchParams(url?.search); | ||
|
|
||
| if ((!oldParams.has(notRefreshingParam) && newParams.has(notRefreshingParam)) || (oldParams.has(notRefreshingParam) && !newParams.has(notRefreshingParam))) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This confirms that the modal parameter was added or removed from the params, but it doesn't prove that modal is the only param that changed.
What if we say, "Don't reload the page if nothing changed other than the modal param"? Because URLSearchParams should preserve parameter order and render the query strings consistently, I think we can say that like this:
| const notRefreshingParam = 'modal'; | |
| const oldParams = new URLSearchParams(prevUrl?.search); | |
| const newParams = new URLSearchParams(url?.search); | |
| if ((!oldParams.has(notRefreshingParam) && newParams.has(notRefreshingParam)) || (oldParams.has(notRefreshingParam) && !newParams.has(notRefreshingParam))) { | |
| return; | |
| } | |
| const oldParams = new URLSearchParams(prevUrl?.search); | |
| const newParams = new URLSearchParams(url?.search); | |
| oldParams.delete('modal'); | |
| newParams.delete('modal'); | |
| if (oldParams.toString() === newParams.toString()) { | |
| return; | |
| } |
That way, we ignore changes to the modal param while reloading for all other param changes.
There was a problem hiding this comment.
What if we say, "Don't reload the page if nothing changed other than the
modalparam"?
😄 sorry, that was always the intent here. It didn't need restated, but it was intended to be in contrast to the old logic, which actually said "Don't reload the page if modal was in one set but not the other."
There was a problem hiding this comment.
If you received the previous comment via email, please ignore it.
I had a coffee and realized that this is a good fix/improvement. 😅
Changes ready for review.
brandonpayton
left a comment
brandonpayton
left a comment
There was a problem hiding this comment.
If we can fix this:
https://github.com/WordPress/wordpress-playground/pull/1908/files#r1823551871
The rest looks good to me and tests well. Thank you!
| // Lean on the Query API parameters and the Blueprint API to | ||
| // create the new site. | ||
| const url = new URL(window.location.href); | ||
| const newUrl = new URL(window.location.href); |
| ); | ||
| closeModal(); | ||
| onImported?.(details); | ||
| closeModal(); |
There was a problem hiding this comment.
This seems like a good change because we'll have a chance to notice the modal remaining open if an error is thrown.
|
|
||
| export const usePrevious = <T>(value: T): T | undefined => { | ||
| const ref = useRef<T>(); | ||
| useEffect(() => { |
There was a problem hiding this comment.
This is a fun use of useEffect to get the previous value. Nice!
| const newParams = new URLSearchParams(url?.search); | ||
| oldParams.delete(notRefreshingParam); | ||
| newParams.delete(notRefreshingParam); | ||
| if (oldParams.toString() === newParams.toString()) { |
There was a problem hiding this comment.
It turns out that this prevents us from automatically creating a temporary site when no other site is selected.
So it looks like, we need to add another condition:
Only avoid reloading the page when there is already an active Playground.
I suppose this makes sense. :)
There was a problem hiding this comment.
Would you mind if I just make that change and land this PR?
There was a problem hiding this comment.
Would you mind if I just make that change and land this PR?
I meant to add: Please also feel free to do it. Whatever you want. That would be fine too. :)
There was a problem hiding this comment.
Done! c06b09b
Once the tests pass, let's go ahead and merge this.
|
Thanks for your contribution, @ajotka! |
3 participants
Motivation for the change, related issues
Issue #1902
Implementation details
Import & Export from Github causes reloading the playground even before accept this step.
I know that it is because of adding new params in the URL.
So I kept functionality to fire those modal via URL, but I also added logic to open those modals without adding params.
Testing Instructions (or ideally a Blueprint)
Also you can check, that opening on slug is still working (but with reloading on closing popup):
http://127.0.0.1:5400/website-server/?modal=github-import