-
Notifications
You must be signed in to change notification settings - Fork 270
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
Move WordPress & Gutenberg PR Preview to Playground website #1938
Conversation
@adamziel Tell me, due to moving this functionality to Playground, should we remove the wordpress.html and gutenberg.html files? FYI, I'm setting this PR as Draft for now, because I'm waiting for the changes to be accepted with other modals. After implementing those changes, I'll add this refactor of all modals -> headers and action buttons. |
Good question! There are existing links to those files out there so adding a redirect to custom-redirects-lib.php seems like the best option. Looping in @brandonpayton.
One of them is now merged 🎉 And the other one has a last bit of pending feedback. I'm happy to merge once it's addressed. Thank you so much for contributing! |
Hi @ajotka! We could add a redirect for https://playground.wordpress.net/?state=github-export If the PR Preview modals were controlled in a similar way, we could add redirects to open them. Here is the code that controls the GitHub Export modal visibility: wordpress-playground/packages/playground/website/src/github/github-export-form/modal.tsx Lines 7 to 32 in 65c4a92
I'm not sure it's great for each modal to control its own visibility, but it should work OK for now. |
Ah, I see how we are controlling other modal visibility via redux state. I think we'll need to have a way to link to certain modals or initialize that redux state if we want to redirect the old |
Oh, good! I see how #1908 should make all modals addressable via query param: At least it looks that way. I haven't tested it. If modals are indeed addressable via query param after #1908, we should have no problem adding redirects after this PR is merged. I added #1969 to track this. |
# Conflicts: # packages/playground/website/src/components/layout/index.tsx
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.
Thank you for all the changes here @ajotka! I really appreciate that you were able to get rid of some of our custom code and replaced it with existing components.
I left a bunch of comments, but most are nitpicks and suggestions.
packages/playground/website/src/components/browser-chrome/index.tsx
Outdated
Show resolved
Hide resolved
packages/playground/website/src/components/site-manager/blueprints-panel/index.tsx
Outdated
Show resolved
Hide resolved
If we want to use an older, safer version instead, we could declare that in a single place and use that.
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.
I made a few changes in response to @bgrgicak's feedback and believe this is ready to merge.
@bgrgicak, I tried to address many of your comments, but there is some feedback regarding the quality of the code this PR migrated from files like wordpress.html that I left alone because it seemed less risky. We can take another pass at that code if we want in a follow-up PR.
@ajotka, thanks for this PR and all your work on it!
packages/playground/website/src/components/browser-chrome/index.tsx
Outdated
Show resolved
Hide resolved
packages/playground/website/src/components/site-manager/blueprints-panel/index.tsx
Outdated
Show resolved
Hide resolved
packages/playground/website/src/github/github-oauth-guard/index.tsx
Outdated
Show resolved
Hide resolved
packages/playground/website/src/github/github-export-form/form.tsx
Outdated
Show resolved
Hide resolved
Note: There are e2e test failures, but those appear to be the same that are currently failing on trunk. We need to fix those under a separate PR. |
Great collaboration here, thank you @ajotka and @brandonpayton! @brandonpayton – I just noticed the GitHub modal lost its styles – is this related to this PR by any chance? |
@adamziel I figured it would be due to this PR, but it turned out to be an earlier commit.
|
Ah, that change added a CSS reset for the web app. That would do it. 😅 |
Motivation for the change, related issues
#1655 (comment)
I moved wordpress.html and gutenberg.html to Playground itself.
By the way, I also made a mini appearance refactor for the modal, based on Figma. And I used components from WordPress packages.
Implementation details
Testing Instructions (or ideally a Blueprint)
Homepage buttonfuture Logo placeIt is also possible to open modal via URL params:
/?modal=preview-pr-wordpress or /?modal=preview-pr-gutenberg