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

fix(design-system): singleton prompt #11352

Merged
merged 9 commits into from
Feb 13, 2025
Merged

fix(design-system): singleton prompt #11352

merged 9 commits into from
Feb 13, 2025

Conversation

fPolic
Copy link
Contributor

@fPolic fPolic commented Feb 6, 2025

What

  • add a flag to disable rendering multiple prompts on a page

Why

  • pressing "r" when a prompt is already open would stack additional prompts

CLOSES SUP-802

Copy link

changeset-bot bot commented Feb 6, 2025

⚠️ No Changeset found

Latest commit: 9c59bce

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Feb 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 13, 2025 9:25am
6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Feb 13, 2025 9:25am
api-reference-v2 ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2025 9:25am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2025 9:25am
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2025 9:25am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2025 9:25am
resources-docs ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2025 9:25am

@fPolic fPolic marked this pull request as ready for review February 7, 2025 09:13
@fPolic fPolic requested review from a team as code owners February 7, 2025 09:13
@fPolic fPolic requested review from sradevski, riqwan, mrnbpt, kasperkristensen and olivermrbl and removed request for sradevski and mrnbpt February 7, 2025 09:13
const prompt = async (props: UsePromptProps): Promise<boolean> => {
if (singleton && flag.current) {
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, but should we perhaps just change it so this is the default behaviour, without needing to do singleton: true? I can't really come up with any cases where you would ever want to render multiple prompts since they stack on top of each other, making everyone except the top one visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes more sense for this to be the default. Will push a change.

# Conflicts:
#	packages/admin/dashboard/src/routes/api-key-management/api-key-management-detail/components/api-key-sales-channel-section/api-key-sales-channel-section.tsx
@fPolic fPolic changed the title fix(dashboard): singleton prompt fix(design-system): singleton prompt Feb 10, 2025
let open = true
if (isPromptActive.current) {
// If the prompt is already active, return the current promise
return currentPromptPromise.current!
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of returning false we return the active promise if prompt is called again while the prompt is already active.

@kodiakhq kodiakhq bot merged commit 4e6372b into develop Feb 13, 2025
23 checks passed
@kodiakhq kodiakhq bot deleted the fix/prompt-singleton branch February 13, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants