Fix: Import & Export from Github causes reloading the playground even before accept this step. by ajotka · Pull Request #1908 · WordPress/wordpress-playground

ajotka · 2024-10-16T11:28:13Z

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)

open http://127.0.0.1:5400/website-server/
Go to playground settings
Click 3 dots next to "Homepage" button
Click export or import from GitHub
Modal should appear without reloading

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

ajotka · 2024-10-22T12:06:10Z

@adamziel ready to check

ajotka · 2024-10-30T13:51:40Z

@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.

brandonpayton · 2024-10-30T22:48:52Z

+			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;
+			}


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:

Suggested change

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.

What if we say, "Don't reload the page if nothing changed other than the modal param"?

😄 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."

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.

😄 all good!

brandonpayton

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!

brandonpayton · 2024-10-31T02:14:25Z

 			// 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);


brandonpayton · 2024-10-31T02:35:55Z

 					);
-					closeModal();
 					onImported?.(details);
+					closeModal();


This seems like a good change because we'll have a chance to notice the modal remaining open if an error is thrown.

brandonpayton · 2024-10-31T03:28:53Z

+
+export const usePrevious = <T>(value: T): T | undefined => {
+	const ref = useRef<T>();
+	useEffect(() => {


This is a fun use of useEffect to get the previous value. Nice!

brandonpayton · 2024-10-31T15:31:31Z

+			const newParams = new URLSearchParams(url?.search);
+			oldParams.delete(notRefreshingParam);
+			newParams.delete(notRefreshingParam);
+			if (oldParams.toString() === newParams.toString()) {


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. :)

Would you mind if I just make that change and land this PR?

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. :)

@brandonpayton go ahead! Thanks :)

Done! c06b09b

Once the tests pass, let's go ahead and merge this.

brandonpayton · 2024-10-31T18:32:31Z

Thanks for your contribution, @ajotka!

ajotka added 5 commits October 16, 2024 12:26

Import Github modal without reloading

50878bf

Export Github without reloading

a62d862

keep slug opens modal functionality

b2a9515

lint

ab78d44

Merge branch 'trunk' into 1902

2bceaf1

adamziel reviewed Oct 16, 2024

View reviewed changes

Comment thread packages/playground/website/src/github/github-export-form/modal.tsx Outdated

ajotka added 3 commits October 22, 2024 08:06

Merge remote-tracking branch 'origin/trunk' into 1902

ccdb830

Merge remote-tracking branch 'origin/trunk' into 1902

192b7f8

remove signal from github modals

60ffe8a

ajotka requested a review from a team as a code owner October 22, 2024 11:34

adamziel reviewed Oct 23, 2024

View reviewed changes

Comment thread packages/playground/website/src/components/layout/index.tsx

adamziel reviewed Oct 23, 2024

View reviewed changes

Comment thread packages/playground/website/src/components/layout/index.tsx Outdated

adamziel reviewed Oct 23, 2024

View reviewed changes

Comment thread packages/playground/website/src/github/github-export-form/modal.tsx

adamziel added the Needs Author's Reply label Oct 29, 2024

Merge remote-tracking branch 'origin/trunk' into 1902

b6a140d

adamziel mentioned this pull request Oct 30, 2024

Web app redesign #1655

Closed

ajotka added 2 commits October 30, 2024 12:23

fixes after CR

71cc2e3

If only the 'modal' parameter changes in url, don't reload the page

b5f3355

adamziel removed the Needs Author's Reply label Oct 30, 2024

brandonpayton mentioned this pull request Oct 30, 2024

Move WordPress & Gutenberg PR Preview to Playground website #1938

Merged

7 tasks

brandonpayton reviewed Oct 30, 2024

View reviewed changes

brandonpayton approved these changes Oct 31, 2024

View reviewed changes

ajotka added 2 commits October 31, 2024 09:24

Merge remote-tracking branch 'origin/trunk' into 1902

419deb7

Brandon suggestion

02932bb

brandonpayton reviewed Oct 31, 2024

View reviewed changes

Fix reload prevention to allow initial temp site creation

c06b09b

brandonpayton merged commit 1e51248 into WordPress:trunk Oct 31, 2024

ajotka mentioned this pull request Nov 1, 2024

Temporary playground sidebar when there is no Playground #1973

Open

Conversation

ajotka commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for the change, related issues

Implementation details

Testing Instructions (or ideally a Blueprint)

Uh oh!

Uh oh!

ajotka commented Oct 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajotka commented Oct 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonpayton Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajotka Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonpayton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonpayton commented Oct 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ajotka commented Oct 16, 2024 •

edited

Loading

brandonpayton Oct 30, 2024 •

edited

Loading

ajotka Oct 31, 2024 •

edited

Loading