Skip to content

chore: refactor dialogs #3935

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

Merged
merged 5 commits into from
Sep 7, 2022
Merged

chore: refactor dialogs #3935

merged 5 commits into from
Sep 7, 2022

Conversation

presleyp
Copy link
Contributor

@presleyp presleyp commented Sep 7, 2022

Just a refactor to lay the groundwork for #3384.

  • put all dialogs in one directory
  • repurpose the one-off component for deleting a workspace into a general component for deleting things, used for templates and workspaces (later this can become a component that asks you to type in the name of what you want to delete, for extra safety)

I've checked that this leaves the delete terminal dialog in a good state but I'm blocked on checking the delete workspace dialog right now.

@presleyp presleyp requested a review from a team as a code owner September 7, 2022 17:51
@presleyp presleyp requested review from Kira-Pilot and BrunoQuaresma and removed request for a team September 7, 2022 17:51

export interface DeleteDialogProps {
isOpen: boolean
handleConfirm: () => void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not keep using the onSomeAction pattern? I think this is the most used one from libs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DeleteWorkspaceDialog component used handle, but I'd like it to be consistent so I'll switch it

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

I'm ok with the changes. Just two minor stuff:

  • A question about the "pattern" that we want to use for event handlers
  • Idk but the diffs on Storybook don't look correct - not showing buttons. Idk if it is something fixable so I will let the approval to you

@presleyp
Copy link
Contributor Author

presleyp commented Sep 7, 2022

@BrunoQuaresma yeah I've seen this storybook issue enough times now that I'm not concerned that it's a problem in the actual app, but I'll have to try to figure out what the problem is.

@presleyp presleyp merged commit 2a085d1 into main Sep 7, 2022
@presleyp presleyp deleted the safe-delete/presleyp/3384 branch September 7, 2022 21:04
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.

2 participants