Skip to content
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

Support users choosing how to handle URLs for sites that do not exist #2059

Merged
merged 22 commits into from
Dec 5, 2024

Conversation

brandonpayton
Copy link
Member

Motivation for the change, related issues

We want to be able to address specific Playgrounds via slug, even if the addressed Playground does not yet exist.

We'd rather not presume and store a Playground in a user's browser storage without their consent, so as a compromise, we can prompt the user and let them choose whether to store a site with the specified slug.

Related to #2037 cc @akirk

Implementation details

This PR adds support for a if-stored-site-missing=prompt query param that can be used alongside a site-slug=<slug> query param. If both params are provided and a site with the specified slug does not exist, we prompt the user and ask what they would like to do.

Screenshot 2024-12-04 at 9 59 20 PM

Testing Instructions (or ideally a Blueprint)

Note that when choosing to continue with the temporary site, it is currently intentional that the original query params are not cleared. The reason is that if we are wanting to target an specific, possibly-non-existent slug, perhaps the user should be prompted with each page refresh.

@brandonpayton brandonpayton added the [Type] Enhancement New feature or request label Dec 5, 2024
@brandonpayton brandonpayton requested a review from a team December 5, 2024 03:09
@brandonpayton
Copy link
Member Author

Please feel free to adjust the language or tweak the behavior here.

One thing that would be good to adjust yet is to wait for the temporary site to load before displaying the modal. Otherwise, the modal will show before it is usable and also obscure the progress indicator.

@brandonpayton
Copy link
Member Author

One thing that would be good to adjust yet is to wait for the temporary site to load before displaying the modal. Otherwise, the modal will show before it is usable and also obscure the progress indicator.

Done.

Copy link
Collaborator

@bgrgicak bgrgicak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandonpayton I left a few suggestions.

Let's also make sure we document this before merging the PR.

Comment on lines +56 to +57
const promptIfSiteMissing =
url.searchParams.get('if-stored-site-missing') === 'prompt';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this extra query argument? Is there any harm in just prompting the user by default?

As a user, I prefer fewer query arguments to remember and type. Also, showing the modal is a reasonable default.

Copy link
Collaborator

@adamziel adamziel Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without that parameter, users will see the modal if they delete one of their Playgrounds and then visit a stale ?slug= link in from their browser history. The extra param ensures the modal is only visible when consent was the intention in the first place.

@adamziel
Copy link
Collaborator

adamziel commented Dec 5, 2024

LGTM! One final note – let's just save it in the browser storage without showing an extra dropdown.

CleanShot 2024-12-05 at 16 24 33@2x

@brandonpayton
Copy link
Member Author

Thank you for your feedback, @bgrgicak and @adamziel!

I appreciate the language improvements and the rest of the suggestions. I added documentation for the new query param and believe all the other suggestions have been addressed as well.

@brandonpayton
Copy link
Member Author

In the docs, the secondary button comes first when side-by-side in left-to-right mode, but it comes last in vertical, top-to-bottom orientation. So I set up the Flex component so the buttons would be side-by-side by default but reverse order when wrapped.

Screenshot 2024-12-05 at 12 00 25 PM

Screenshot 2024-12-05 at 12 00 37 PM

@brandonpayton brandonpayton merged commit 00438ae into trunk Dec 5, 2024
10 checks passed
@brandonpayton brandonpayton deleted the allow-user-to-choose-non-existent-site-handling branch December 5, 2024 17:44
akirk added a commit to akirk/playground-step-library that referenced this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants