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

Add modal dialog to Storybook #5496

Merged
merged 5 commits into from
Jan 14, 2025
Merged

Add modal dialog to Storybook #5496

merged 5 commits into from
Jan 14, 2025

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Jan 14, 2025

For Wolasi's question - and to hopefully clarify the distinction between the different APIs (modal vs dialog), and why they're separate in the first place (popovers can also be modal, although usually without the grey overlay).

@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Jan 14, 2025
@Vinnl Vinnl requested review from codemist and flozia January 14, 2025 09:06
@Vinnl Vinnl self-assigned this Jan 14, 2025
Copy link

Comment on lines 48 to 63
<>
<p>
This is modal content; note that it&apos;s not possible to
interact with the content behind the modal overlay. Here&apos;s
a button to close the modal: &nbsp;
</p>
<Button variant="primary" onPress={() => modalState.close()}>
Close modal
</Button>
</>
)}
</ModalOverlay>
)}
</>
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious why we have a modal without dialog variant here, I don't think we're using this in the app?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're not, but I wanted to make sure people understood what you get if you use just the modal overlay, to clarify what the dialog is and what the modal is.

Copy link
Collaborator

@codemist codemist Jan 14, 2025

Choose a reason for hiding this comment

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

Gotcha. Could we add a comment somewhere that this isn't being used in the app, since it is a UI library we wouldn't want to give off the assumption that it's an intentional FE component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good callout, I added 7a94a77 which calls it out right in the story, what do you think?

I'll see if I get another E2E failure, they seem to be flaky today - I wouldn't expect this PR to affect the appsAndSerices button 😅

@codemist
Copy link
Collaborator

There seems to be an e2e test failure with the appsAndServices button?

Copy link
Collaborator

@flozia flozia left a comment

Choose a reason for hiding this comment

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

Thanks for adding the story.

@Vinnl Vinnl merged commit f33c7a0 into main Jan 14, 2025
15 of 16 checks passed
@Vinnl Vinnl deleted the modal-dialog-stories branch January 14, 2025 16:15
Copy link

Cleanup completed - database 'blurts-server-pr-5496' destroyed, cloud run service 'blurts-server-pr-5496' destroyed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants