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

Ask for confirmation before deleting users #31

Merged
merged 5 commits into from
Apr 23, 2024

Conversation

AAJELLAL
Copy link
Contributor

No description provided.

Signed-off-by: AAJELLAL <ali.ajellalod@gmail.com>
…o add_confirmation_before_deleting_users

# Conflicts:
#	src/translations/en.json
#	src/translations/fr.json
Signed-off-by: AAJELLAL <ali.ajellalod@gmail.com>
src/pages/users/UsersPage.tsx Outdated Show resolved Hide resolved
}, [gridContext, rowsSelection, snackError]);
const deleteUsersDisabled = useMemo(
() => rowsSelection.length <= 0,
[rowsSelection.length]
);

const handleDeletion = (value: boolean) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can directly call show(DeletionDialog) instead of handleDeletion. It would be more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always prefer to use a separate method instead of calling the setXX directly. This way it's easy to maintain and we can add other verification as well if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it makes sense too.

src/pages/users/UsersPage.tsx Outdated Show resolved Hide resolved
<FormattedMessage id="ok" />
</Button>
</DialogActions>
</Dialog>
Copy link
Contributor

Choose a reason for hiding this comment

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

UserPage was a big componant ... and is now more bigger.
We should split it in several part, like ProfilePages. Maybe not in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed the component is too big. However the file naming must be harmonized.
Example: UsersPage.tsx and profiles-page.tsx

Copy link
Contributor

Choose a reason for hiding this comment

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

the global naming in GridSuite is more my-component.tsx rather than MyComponent.tsx

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thanks for DeleteUserDialog

src/translations/fr.json Outdated Show resolved Hide resolved
src/translations/en.json Outdated Show resolved Hide resolved
@dbraquart dbraquart self-requested a review April 19, 2024 15:39
Copy link
Contributor

@dbraquart dbraquart left a comment

Choose a reason for hiding this comment

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

code: ok, just minor remarks about naming
tests: works fine. Minor issue: en/fr titles are different.

Signed-off-by: AAJELLAL <ali.ajellalod@gmail.com>
@AAJELLAL AAJELLAL requested a review from dbraquart April 23, 2024 13:10
Copy link
Contributor

@dbraquart dbraquart left a comment

Choose a reason for hiding this comment

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

code: ok (except a missing header)
tests: ok

src/pages/users/delete-user-dialog.tsx Show resolved Hide resolved
Signed-off-by: AAJELLAL <ali.ajellalod@gmail.com>
@AAJELLAL AAJELLAL requested a review from dbraquart April 23, 2024 14:38
@AAJELLAL AAJELLAL merged commit 1fb9965 into main Apr 23, 2024
2 checks passed
@AAJELLAL AAJELLAL deleted the add_confirmation_before_deleting_users branch April 23, 2024 14:41
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