-
Notifications
You must be signed in to change notification settings - Fork 4k
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(dashboard): Workflow saving status on toast and multiple toasts #6972
Conversation
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
onSuccess, | ||
onError, | ||
export const useUpdateWorkflow = ( | ||
options?: UseMutationOptions<WorkflowResponseDto, unknown, Parameters<typeof updateWorkflow>[0]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should not expose the react-query options as we will be tied by it's interface, but rather expose ours with the needed functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I wouldn't create a useUpdateWorkflow
hook in general. If we create one hook per mutation/query, we are already tied by its interface. I don't see any difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the difference is that we will be free to change the implementation of the hook to whatever we want if we decide
31b303c
to
789d8c9
Compare
What changed? Why was the change needed?
Screenshots
Screen.Recording.2024-11-12.at.5.46.53.PM.mov
Expand for optional sections
Related enterprise PR
Special notes for your reviewer