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

refactor(frontend): update deleteDialog logic #588

Closed

Conversation

yassinedorbozgithub
Copy link
Collaborator

@yassinedorbozgithub yassinedorbozgithub commented Jan 18, 2025

Motivation

The main motivation of this PR is to refactor delete useDialog, delete component and to introduce the new callbackHandler.

Fixes #587

Screenshots

image
image
image
image

Type of change:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have performed a self-review of my own code
  • New and existing unit tests pass locally with my changes

@yassinedorbozgithub yassinedorbozgithub added the enhancement New feature or request label Jan 18, 2025
@yassinedorbozgithub yassinedorbozgithub self-assigned this Jan 18, 2025
@yassinedorbozgithub yassinedorbozgithub linked an issue Jan 18, 2025 that may be closed by this pull request
18 tasks
@yassinedorbozgithub yassinedorbozgithub changed the title 587 issue refactor delete dialog logic refactor(frontend): update deleteDialog logic Jan 18, 2025
frontend/src/hooks/useDialog.tsx Outdated Show resolved Hide resolved
frontend/src/hooks/useDialog.tsx Outdated Show resolved Hide resolved
frontend/src/hooks/useDialog.tsx Outdated Show resolved Hide resolved
frontend/src/hooks/useDialog.tsx Outdated Show resolved Hide resolved
frontend/src/hooks/useDialog.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@marrouchi marrouchi 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 thinking about refactoring even more. Maybe we can have a single useDelete() that accepts an array of ids. Within the useMutation() it would decide to call either bulk delete or just delete. Let me know what you think.

@yassinedorbozgithub
Copy link
Collaborator Author

I'm thinking about refactoring even more. Maybe we can have a single useDelete() that accepts an array of ids. Within the useMutation() it would decide to call either bulk delete or just delete. Let me know what you think.

It make a full sense, it's a good idea, happy to implement those enhancements 🚀

frontend/src/app-components/dialogs/DeleteDialog.tsx Outdated Show resolved Hide resolved
frontend/src/app-components/dialogs/DeleteDialog.tsx Outdated Show resolved Hide resolved
frontend/src/app-components/dialogs/DeleteDialog.tsx Outdated Show resolved Hide resolved
frontend/src/components/categories/index.tsx Outdated Show resolved Hide resolved
frontend/src/app-components/dialogs/DeleteDialog.tsx Outdated Show resolved Hide resolved
@yassinedorbozgithub yassinedorbozgithub marked this pull request as draft January 24, 2025 08:34
@IkbelTalebHssan IkbelTalebHssan self-requested a review January 24, 2025 14:11
Copy link
Collaborator

@IkbelTalebHssan IkbelTalebHssan left a comment

Choose a reason for hiding this comment

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

LGTM ✔️ 🚀

@yassinedorbozgithub yassinedorbozgithub marked this pull request as ready for review January 25, 2025 14:01
Comment on lines 31 to 32
onDeleteError = () => {},
onDeleteSuccess = () => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onDeleteError = () => {},
onDeleteSuccess = () => {},
onError = () => {},
onSuccess = () => {},

Since it's the delete dialog, it's kindof repetitive to indicate it again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main motivation of this naming is to avoid overriding the onError attribute of the dialog component

@yassinedorbozgithub
Copy link
Collaborator Author

Closed in favor of using useDialogs hook
Implementation example : #662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🤔 [ISSUE] - refactor dialogs
3 participants