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.

Sign up for GitHub

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

Feat: implement DeleteGroupDialog #248

Merged
merged 7 commits into from
Feb 27, 2024
Merged

Feat: implement DeleteGroupDialog #248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9849cc3
feat: implement deleteGroupConfirmDialog
kanta1207 Feb 21, 2024
d12a2d2
Merge branch 'develop' into feature/244-delete-group-confirm-dialog-c…
kanta1207 Feb 27, 2024
b40824e
feat: re-implement delete group dialog with new file structure
kanta1207 Feb 27, 2024
ef85449
fix: out of scope, env variable name
kanta1207 Feb 27, 2024
9c5e7d2
fix: restructure to close dropdown menu on dialog close
kanta1207 Feb 27, 2024
00e3444
fix: remove unnecessary asChild in DropdownMenuItem
kanta1207 Feb 27, 2024
0e04270
fix: close dropdown on closing dialog
kanta1207 Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions ...d/GroupCardContent/GroupCardDropdownMenuTriggerButton/DeleteGroupConfirmDialogContent.tsx
View file
Open in desktop
Copy link
Member

@nick-y-ito nick-y-ito Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From my perspective, naming this component DeleteGroupDialogContent (without "confirmation") is understandable enough. The name DeleteGroupConfirmDialogContent sounds to me like a "second" dialog to confirm the deletion. You can decide it. However, since our design has multiple delete dialogs, we should prioritize consistency across the project. You can decide :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed the file name to DeleteGroupDialog!

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import {
Button,
DialogBody,
DialogClose,
DialogContent,
DialogFooter,
DialogHeader,
DialogTitle,
} from '@/components/ui';

import React from 'react';

export const DeleteGroupConfirmDialogContent = () => {
return (
<DialogContent>
<DialogHeader>
<DialogTitle>Delete Group</DialogTitle>
</DialogHeader>
<DialogBody>
<p>Are you sure you want to delete group?</p>
</DialogBody>
<DialogFooter>
<DialogClose asChild>
<Button variant="cancel" size="sm">
Cancel
</Button>
</DialogClose>
<Button
variant="error"
size="sm"
onClick={() => {
alert('delete');
}}
>
Delete
</Button>
</DialogFooter>
</DialogContent>
);
};
16 changes: 10 additions & 6 deletions ...Card/GroupCardContent/GroupCardDropdownMenuTriggerButton/GroupCardDropdownMenuContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { DeleteIcon, PenIcon } from '@/assets/images/icons';
import {
DialogTrigger,
DropdownMenuButton,
DropdownMenuButtonIcon,
DropdownMenuButtonText,
Expand Down Expand Up @@ -28,12 +29,15 @@ export const GroupCardDropdownMenuContent: FC<IGroupCardDropdownMenuContentProps
</DropdownMenuButton>
</DropdownMenuItem>
<DropdownMenuItem asChild>
<DropdownMenuButton onClick={() => {}}>
<DropdownMenuButtonIcon>
<Icon icon={DeleteIcon} size={5} color="danger" />
</DropdownMenuButtonIcon>
<DropdownMenuButtonText>Delete</DropdownMenuButtonText>
</DropdownMenuButton>
{/* Wrapping delete button with DialogTrigger to open the dialog in parent component*/}
<DialogTrigger asChild>
<DropdownMenuButton>
<DropdownMenuButtonIcon>
<Icon icon={DeleteIcon} size={5} color="danger" />
</DropdownMenuButtonIcon>
<DropdownMenuButtonText>Delete</DropdownMenuButtonText>
</DropdownMenuButton>
</DialogTrigger>
</DropdownMenuItem>
</DropdownMenuContent>
);
Expand Down
23 changes: 14 additions & 9 deletions ...nts/GroupCardList/GroupCard/GroupCardContent/GroupCardDropdownMenuTriggerButton/index.tsx
View file
Open in desktop
Copy link
Member Author

@kanta1207 kanta1207 Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This file has changed just because I removed FC type.

I did the change only to this file for now because the file was in this PR's scope. Maybe I'll refactor rest of FC types in other files later in different scope.

Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
'use client';
import { MenuKebabIcon } from '@/assets/images/icons';
import { Button, DropdownMenu, DropdownMenuTrigger, Icon } from '@/components/ui';
import { Button, DialogRoot, DropdownMenu, DropdownMenuTrigger, Icon } from '@/components/ui';

import { FC } from 'react';

import { DeleteGroupConfirmDialogContent } from './DeleteGroupConfirmDialogContent';
import { GroupCardDropdownMenuContent } from './GroupCardDropdownMenuContent';

interface IGroupCardMenuButtonProps {
Expand All @@ -14,13 +15,17 @@ export const GroupCardDropdownMenuTriggerButton: FC<IGroupCardMenuButtonProps> =
handleRenameClick,
}) => {
return (
<DropdownMenu>
<DropdownMenuTrigger asChild>
<Button variant="ghost" className="w-12">
<Icon icon={MenuKebabIcon} size={4.5} />
</Button>
</DropdownMenuTrigger>
<GroupCardDropdownMenuContent handleRenameClick={handleRenameClick} />
Copy link
Member Author

Choose a reason for hiding this comment

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

Because I separated GroupCardDropdownMenuContent from trigger as a different component,
I ended up having to pass onDropdownMenuClose prop from this file, and it's props drilling among 4 layers.

(GroupCardDropdownMenuTriggerButton => GroupCardDropdownMenuContent => DeleteGroupDialogTriggerButton => DeleteGroupDialogContent)

The directory structure itself is reasonable one tho, let me know if you feel we have to do something for this props drilling. I personally chose to keep this directory structure for now.

Copy link
Member

@nick-y-ito nick-y-ito Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I personally chose to keep this directory structure for now.

I agree. I think it's acceptable for now.

</DropdownMenu>
// Wrapping dialog menu with DialogRoot. Trigger is in GroupCarDropdownMenuContent
Copy link
Member

@nick-y-ito nick-y-ito Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry, but could you please clarify this comment? It's a bit difficult to understand what it's trying to say.

I could be like the following if my understanding is correct:

The <DropdownMenu> must be wrapped with the <DialogRoot> to prevent the dialog from closing unintentionally when the dropdown menu is closed. The trigger for the dialog can be found in the <GroupCarDropdownMenuContent>.

Copy link
Member Author

Choose a reason for hiding this comment

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

This issue was addressed by adjusting DeleteGroupDialog to follow how it's implemented in DeleteFoodDialog.

<DialogRoot>
<DropdownMenu>
<DropdownMenuTrigger asChild>
<Button variant="ghost" className="w-12">
<Icon icon={MenuKebabIcon} size={4.5} />
</Button>
</DropdownMenuTrigger>
<GroupCardDropdownMenuContent handleRenameClick={handleRenameClick} />
</DropdownMenu>
<DeleteGroupConfirmDialogContent />
</DialogRoot>
);
};
Loading