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

Replace Modal.isOpen with controlled boolean #422

Merged
merged 5 commits into from
Nov 8, 2023

Conversation

mcous
Copy link
Member

@mcous mcous commented Nov 8, 2023

Overview

When writing the Modal component for prime-core, I suggested we ditch events in favor of the two-way data binding of an isOpen store. After some usage in the app, we've discovered this is a pretty awkward API.

This PR switches the modal back to a plain, controlled isOpen: boolean prop and a close event, which should be more predictable. As a result, this means the modal is no longer capable of closing itself; it must emit the close event and the parent must chose to close it.

I think there are continuing improvements to be made here, especially given the awkwardness of combining the close event with any buttons placed in the primary and secondary slots, but I think this is a step in the right direction.

This is a breaking change to prime-core. See the companion PR in the app: https://github.com/viamrobotics/app/pull/2873

Review requests

  • Read through unit test updates
  • Test out the playground
  • Check storybook

@mcous mcous requested review from ehhong, Oabraham1 and anjinai November 8, 2023 15:35
Copy link
Member

@ehhong ehhong left a comment

Choose a reason for hiding this comment

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

followed review requests and everything lgtm! the modal is not really visible in storybook but the code snippets look good

Copy link
Member

@anjinai anjinai left a comment

Choose a reason for hiding this comment

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

lgtm too - Followed review requests and have the same comment as emily re: the modal not being visible in storybook.

@mcous mcous merged commit e7bbcb0 into viamrobotics:main Nov 8, 2023
@mcous mcous deleted the no-modal-store branch November 8, 2023 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants