[Feature] Ability to Delete a Flow from the Index Page #1
Conversation
| const mutationOptions: MutationOptions = useMemo( | ||
| () => ({ | ||
| onSuccess: () => { | ||
| toast({ title: "Flow Deleted" }); |
There was a problem hiding this comment.
This toast could use better styling to have parity with the existing delete flow experience, but for the sake of time, just wanted to demonstrate the functionality.
| const { mutate: deleteFlow, isPending } = useMutation( | ||
| deleteFlowMutation(id, mutationOptions), | ||
| ); |
There was a problem hiding this comment.
I noticed a FlowQuery class that has a getter defined for this method.
Seems it is mostly intended to be use by the flows detail page, so went ahead and imported the method directly here as opposed to using the FlowQuery. This is because as more functionality is added to these cells / dropdown menu, probably not the best idea to new up a FlowQuery in every component that is taking an action on the flow.
However, I could see a world where the data coming into these cells is transformed a bit more, possibly leveraging the FlowQuery class. This would likely happen somewhere further upstream, either in the route loader or possibly the flows data-table component
| }: { | ||
| id: string; | ||
| name: string; | ||
| onClose?: () => void; |
There was a problem hiding this comment.
This needs to be passed in so that we can close the menu when "close" or "delete" is clicked. Has to do with this issue radix-ui/primitives#1836
| onSettled: () => { | ||
| return queryClient.invalidateQueries({ | ||
| predicate: (query) => { | ||
| const firstKey = query.queryKey[0]; | ||
| return typeof firstKey === "string" && firstKey.includes("flows"); | ||
| }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Felt this is the easiest way to ensure the cache is updating after taking the delete action. So long as the API is reasonably fast, this is a fine UX when coupled to a loading indicator on the Delete button.
However, for future consideration, there is Optimistic Update support from Tanstack React Query. It's a bit more code and a bit more cognitive overhead though, and I'm not convinced the juice is worth the squeeze in this particular scenario. An option to be aware of and consider though.
There was a problem hiding this comment.
Adds in the AlertDialog from shadcn. We already have a Dialog component, but as discussed in the PR description, the AlertDialog is more appropriate to interrupting a user action and asking for confirmation.
There was a problem hiding this comment.
While this test is fine and somewhat useful, I would probably lean toward a more "integration" style test, where we load up the Flows Index page and navigate through to the menu, clicking the delete, and confirming it works. That would test more of the code and ensure all the pieces work together well, such as the the rendering of the dialog trigger from a dropdown menu, the request action with useMutation (mocked at the network with MSW), etc...
However, there is maybe room for both a more granular unit test on this component and an "integration" style test.
For the sake of time, I stuck with more of a unit test approach since we don't have a Flows page test setup, similar to the Variables Page Test.
In general on this topic, I tend to follow the guidance of the react testing library, and leverage mostly integration style tests though.
| className="bg-red-500 hover:bg-red-400 focus:ring-red-600" | ||
| > | ||
| {deletionIsPending ? ( | ||
| <Loader2 className="w-4 h-4 animate-spin" /> |
There was a problem hiding this comment.
As discussed in a previous comment, this loader is here since we are not doing optimistic updates in this MR.
| @@ -5,3 +5,8 @@ export type JSONValue = | |||
| | Record<string, never> | |||
| | unknown[] | |||
| | null; | |||
|
|
|||
| export type ReactComponentPropsWithClassName<T extends React.ElementType> = | |||
There was a problem hiding this comment.
This was added to make edits in the alert-dialog from shadcn easier.
When copy/pasting there are initially typescript errors, because the composable components of the AlertDialog need a className prop in addition to the radix primitive props.
Seems this will be a common problem whenever importing shadcn components, and this custom type allows a one line change on each component of ReactComponentPropsWithClassName<typeof AlertDialogPrimitive.Action> instead of having to define explicit prop types for each above the component.
j-brenneman
deleted the
feature/ability-to-delete-flow-from-index-page
branch
Description
As part of the Prefect Replatform the UI Issue, this MR ....
<ConfirmDeleteDialog />)<ConfirmDeleteDialog />to the Flows Index page and enables deleting a flow<ConfirmDeleteDialog />with vitetest and react testing library<ConfirmDeleteDialog />Screenshots and UI/UX Decisions
Component
Notice the subtle differences in styling.
It would be straight forward to remove these differences and create parity, however, I decided to stick with the default styles from shadcn for sake of scope. These types of differences would be a great discussion to have with product/design partners to align on the priority, trade offs and scope of work.
Notice the differences in functionality.
Xicon in top right to close the modal, React component does notThis difference comes from the choice to use an Alert Dialog for the React component, as opposed to a standard dialog. The Alert Dialog is more appropriate to the use case, as it's presentation to the user is an interruption of an action, forcing the user to make a choice (Delete or Cancel). Thus, the removal of the
Xicon, as that is a more passive action to take. This decision to use an Alert Dialog also comes with the benefit of more appropriate role assignment for screen readers (alertdialogvsdialog).Once again, this is a decision to work out with product / design partners.
For more information on the differences between a Messaging Dialog and Alert Dialog, refer to the following ARIA Authoring Practices Guides.
Alert and Message Dialogs Pattern
Dialog (Modal) Pattern
Existing Vue Component
New React Component
User Flow
Screen.Recording.2024-11-18.at.11.50.18.AM.mov
Storybook
Screen.Recording.2024-11-18.at.11.55.03.AM.mov