Feat: implement DeleteGroupDialog #248

kanta1207 · 2024-02-21T20:06:47Z

Overview

Implement DeleteGroupConfirmDialog component.

Changes

Wrap GroupCardDropdownMenu with DialogRoot, and put DeleteGroupConfirmDialog outside of drop down menu because if DialogRoot goes inside of GroupCardDropdownMenuContent, closing the dropdown will also close the dialog.
Add DeleteGroupConfirmDialog component.
Add trigger to open the dialog in GroupCardDropdownMenuContent.

Review points

The trigger is in GroupCardDropdownMenuContent, content is in GroupCardDropdownMenu. Is this component structure complexity okay? If not, not using DialogTrigger to open the dialog can be an another solution.

Screen Captures

Screen.Recording.2024-02-21.at.12.36.52.mov

Assignee Checklist:

The naming convention of the PR title is correct (See the comment at the top of this template)
The base branch is correct (See: Types of Branches)
The branch name follows the Branch Naming Conventions
The correct assignees and reviewers have been designated for this PR
The coding style follows the Coding Style Guide
All the related issues are associated with this PR
All criteria in the associated issues are met (please tick the checkboxes)
My changes do not generate new warnings or errors (especially in the console)

Reviewer Checklist:

Double-check the "Assignee Checklist"
The code follows the generic best practices and our Coding Style Guide
The code is well-commented and easy to understand
The UI changes accurately reflect the provided design

nick-y-ito

@kanta1207
Thank you for researching the issue of how to prevent the dialog from unintentionally closing together when the dropdown menu is closed. I couldn't even find any solution when I tried.

However, I still doubt this approach, although it works here.

Wrap GroupCardDropdownMenu with DialogRoot, and put DeleteGroupConfirmDialog outside of drop down menu because if DialogRoot goes inside of GroupCardDropdownMenuContent, closing the dropdown will also close the dialog.

The reason is that with this approach, we would not be able to have two or more dialogs within the same dropdown menu. Here are the cases we should face shortly:

Review Points

The trigger is in GroupCardDropdownMenuContent, content is in GroupCardDropdownMenu. Is this component structure complexity okay?

I think the complexity is acceptable as long as the comments are appropriately written.

If not, not using DialogTrigger to open the dialog can be an another solution.

I am wondering if not using the DialogTrigger may degrade the accessibility.

We may need to discuss this.

nick-y-ito · 2024-02-22T06:28:44Z

...onents/GroupCardList/GroupCard/GroupCardContent/GroupCardDropdownMenuTriggerButton/index.tsx

-      </DropdownMenuTrigger>
-      <GroupCardDropdownMenuContent handleRenameClick={handleRenameClick} />
-    </DropdownMenu>
+    // Wrapping dialog menu with DialogRoot. Trigger is in GroupCarDropdownMenuContent


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>.

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

nick-y-ito · 2024-02-22T07:25:39Z

...Card/GroupCardContent/GroupCardDropdownMenuTriggerButton/DeleteGroupConfirmDialogContent.tsx

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 :)

Renamed the file name to DeleteGroupDialog!

…omponent

kanta1207 · 2024-02-27T01:35:35Z

...onents/GroupCardList/GroupCard/GroupCardContent/GroupCardDropdownMenuTriggerButton/index.tsx

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.

kanta1207 · 2024-02-27T01:36:33Z

...Card/GroupCardContent/GroupCardDropdownMenuTriggerButton/DeleteGroupConfirmDialogContent.tsx

Renamed the file name to DeleteGroupDialog!

kanta1207 · 2024-02-27T01:38:18Z

...onents/GroupCardList/GroupCard/GroupCardContent/GroupCardDropdownMenuTriggerButton/index.tsx

-      </DropdownMenuTrigger>
-      <GroupCardDropdownMenuContent handleRenameClick={handleRenameClick} />
-    </DropdownMenu>
+    // Wrapping dialog menu with DialogRoot. Trigger is in GroupCarDropdownMenuContent


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

kanta1207 · 2024-02-27T02:35:36Z

src/lib/api/group/client/groupApiClient.client.ts

@@ -122,7 +122,7 @@ export const deleteMember = async (
 ): Promise<Result<undefined, string>> => {
  try {
    await request({
-      url: `${BACKEND_API_DOMAIN}/groups/${groupId}/users/${userId}`,
+      url: `${API_BASE_URL}/groups/${groupId}/users/${userId}`,


Found out thisBACKEND_API_DOMAIN variable (which doesn't exist) is set as base url in here, and caused build error. The functionality shouldn't be also working tho, don't know how it's in develop branch now...

kanta1207 · 2024-02-27T02:37:45Z

@nick-y-ito
Thank you for your review!
I basically followed the way it's implemented in DeleteFoodDialog, and this PR stays focusing on only UI.

Please take a look at the change when you have time 🙏

nick-y-ito

@kanta1207
Thank you for updating! Please kindly take a look at the comments. They are not as crucial as I have to request changes.

However, I suppose the dropdown menu should be closed together when closing the dialog. If you are planning to implement that behavior in the next PR, please never mind :)

If you address this and need my further review, please feel free to ask me again!

nick-y-ito · 2024-02-27T05:03:01Z

...d/GroupCardContent/GroupCardDropdownMenuTriggerButton/GroupCardDropdownMenuContent/index.tsx

+   * This is necessary because closing dropdown menu also closes the dialog unintentionally.
+   * @see {@link https://www.radix-ui.com/primitives/docs/components/dropdown-menu#item}
+   *
+   * @param e The event object


This is a minor thing, though; here is the official way of formatting the @param tag. It is good to be aware of.

/** * @param e - The event object */

https://tsdoc.org/pages/tags/param/

nick-y-ito · 2024-02-27T05:05:14Z

...ton/GroupCardDropdownMenuContent/DeleteGroupDialogTriggerButton/DeleteGroupDialogContent.tsx

+        <DialogTitle>Delete Group</DialogTitle>
+      </DialogHeader>
+      <DialogBody>
+        <p>Are you sure you want to delete group?</p>


"Are you sure you want to delete 'this' group?" would sound slightly better.

nick-y-ito · 2024-02-27T05:05:37Z

...pdownMenuTriggerButton/GroupCardDropdownMenuContent/DeleteGroupDialogTriggerButton/index.tsx

+  Icon,
+} from '@/components/ui';
+
+import React from 'react';


Maybe unnecessary

nick-y-ito · 2024-02-27T05:05:50Z

...ton/GroupCardDropdownMenuContent/DeleteGroupDialogTriggerButton/DeleteGroupDialogContent.tsx

+  DialogTitle,
+} from '@/components/ui';
+
+import React from 'react';


Maybe unnecessary

nick-y-ito

I am sorry, I found an error in the console after I approved. That says:

app-index.js:35 Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

Could you inspect it, please?

delete-container-dropdown-menu-error.mov

kanta1207 · 2024-02-27T06:19:10Z

@nick-y-ito
Thank you for your review!

the dropdown menu should be closed together when closing the dialog

I just forgot to implement the functionality in this PR, so I included it in commit #9c5e7d2.

I am sorry, I found an error in the console after I approved. That says:
app-index.js:35 Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
Could you inspect it, please?

Sorry that I didn't notice the warning in the first place. I should've checked that.
I fixed the issue in the commit #00e3444.
It was because of unnecessary asChild in DropdownMenuItem.

Please kindly take a look at them again on your convenience 🙏

kanta1207 · 2024-02-27T06:30:12Z

...onents/GroupCardList/GroupCard/GroupCardContent/GroupCardDropdownMenuTriggerButton/index.tsx

      <DropdownMenuTrigger asChild>
        <Button variant="ghost" className="w-12">
          <Icon icon={MenuKebabIcon} size={4.5} />
        </Button>
      </DropdownMenuTrigger>
-      <GroupCardDropdownMenuContent handleRenameClick={handleRenameClick} />


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.

I personally chose to keep this directory structure for now.

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

nick-y-ito

@kanta1207
Thank you for updating!

I think the dropdown menu should also be closed when after the [ Delete ] button is clicked.

There is another problem with this behavior, though...
#269

nick-y-ito · 2024-02-27T07:07:24Z

...onents/GroupCardList/GroupCard/GroupCardContent/GroupCardDropdownMenuTriggerButton/index.tsx

      <DropdownMenuTrigger asChild>
        <Button variant="ghost" className="w-12">
          <Icon icon={MenuKebabIcon} size={4.5} />
        </Button>
      </DropdownMenuTrigger>
-      <GroupCardDropdownMenuContent handleRenameClick={handleRenameClick} />


I personally chose to keep this directory structure for now.

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

kanta1207 · 2024-02-27T07:55:53Z

@kanta1207 Thank you for updating!

I think the dropdown menu should also be closed when after the [ Delete ] button is clicked.

There is another problem with this behavior, though... #269

Thank you for your review!
I fixed to close dropdown on closing dialog with delete button clicked! Please take a look again 🙏

nick-y-ito

@kanta1207
Thank you for fixing them promptly! Looks good to go 😄

* Feature: create container feature(UI) in group single page (#256) * feat: implemenet drawer UI" * feat: implement UI of creating container drawer * chore: delete console.log * fix: change the first letter of file name to lower case * fix: add a type to the function parameter * fix: change to self-closing * fix: avoid using type annotation * docs: add docs to interface and function * fix(creategroupdrawercontent.tsx): add param, change to self-closing in whicht it referenced from * Feature: add delete functionality to the members page (#268) * fix: adjust menuIcon and 'Members' in MembersPage * feat: implement popup DropdownMenu when clicked only UI" * feat: implement member delete dialog * refactor: create on click function * feat: disable dialog and dropdown at same time when button clicked * chore: delete unnecessary code * test: set up for deleting-member function * docs: add docs to interfaces and functions * test: change the way of comparison to primitive * test: add schema for validation * docs: add doc to the handling function for delete button * docs: add doc to interface IMemberPageProps * fix: typo * fix: change onClick event to one line * Feature: Add validation to the DELETE food (#266) * feat: add validation to the DELETE food * feat: revise the ID validation for CREATE & UPDATE food * Feature: Adjust environment variables and deploy on Vercel (#267) * refactor: rename env LOCALHOST_URL * refactor: rename env BACKEND_API_DOMAIN * refactor: unify OAuth Domain and Redirect URL * chore: clarify the env.example * fix: gitignore for env * Feature: Create container feature api in group single page (#272) * feat: pass groupId to child * feat: implement fetch function * test: test for postContainer * fix: change Domain variable * test: implement test for action * test: implement test for API error * feat: implement call API when submit * docs: add docs to interface and function * docs: typo * fix: modified variable and put a link in docs to improve readability * docs: add doc for more descriptive * refactor: move createContainer function to top for readability * refactor: change the function name from postContainer to postCreateContaier * fix: typo * refactor: separate container and food test function * Feat: implement DeleteGroupDialog (#248) * feat: implement deleteGroupConfirmDialog * feat: re-implement delete group dialog with new file structure * fix: out of scope, env variable name * fix: restructure to close dropdown menu on dialog close * fix: remove unnecessary asChild in DropdownMenuItem * fix: close dropdown on closing dialog * Docs: Relocate the content of NOTE documents (#276) * refactor: refactor & add TSDocs in Icon.tsx * docs: remove note docs * docs: add Icons sections to the style guide * refactor: rename icon components to improve accessibility * Docs: Improve `README.md` & `CONTRIBUTING.md` (#281) * Feature: Apply a correct link to the back button of each page (#271) * feat: allow the Link component to accept any attributes * feat: correct the back link for foods page * feat: create API client and apply group name to header * refactor: rename GroupSingle * style: add line break * chore: allow leading underscore for variable names * fix: correct env and solve the compile error * Feat : implement delete group api client (#282) * feat: implement delete group api client * refactor: refactor TsDoc comments for existing action functions * feat: add remove group action * refactor: refactor group action file * feat: apply api client to delete dialog component * fix: fix following coding style guide * fix: refactor TsDoc comments to add info * Feature: Rename container UI in group single page (#287) * feat: implement UI of DropdownMenu * feat: implement switching container name to input field * feat: add 1rem padding to right side of input * docs: add docs to interface and function * style: formatting for readability * docs: add docs to the function which it is missing * fix: change default font size of input from ld to md * refactor: change the event props name to onXXX * fix: change type to alias * Feature: Apply design changes to Select.tsx (#249) * style(select.tsx): changed some styles to match the Figma design * style(select.tsx): added a wrapper to check icon to align with the Figma design #127 * style(select.tsx): added className that was accidentally deleted in the previous commit #127 * style(select.tsx): fixed icon name #127 * style(select.tsx): applied style changes to the icon #127 * Feature: Rename container api in group single page (#297) * feat: implement UI of DropdownMenu * feat: implement switching container name to input field * feat: add 1rem padding to right side of input * docs: add docs to interface and function * style: formatting for readability * fix: update Icon name * feat: implement putRenameContainer function * docs: add docs to the function which it is missing * fix: change default font size of input from ld to md * refactor: change the event props name to onXXX * refactor: delete unnecessary file * feat: implement API call * fix: change type to alias * feat: pass containerId to child components * feat: implement test without validation * test: add validation test * docs: add comment on function * style: add lines * docs: add doc to renameContainer functions * style: add lines for readability * fix: update the messages * fix: update the message in test * refactor: change the structure in action for readability * fix: create a gap between each card * fix: modify the position of icon when error appears * fix: typo * fix: change the approach to self-start * Feature: Disable authentication for contributors (#290) * feat: create auth utils * feat: disable login flow * feat: disable toke check when using mock API * feat: update .env.example * feat: copy .env.example to .env.local on `postinstall` * chore: add comments in .env.example * test: fix test for request function * refactor: increase the readability * Feature: Adjust the UI & behavior of the login form (#286) * feat: added google login icon * feat: google login with reac-ui amplify * feat: rm unused code * refactor: remove unused styles in tailwind.config.ts * refactor: define color variables in CSS * feat: add spinner image * feat: create the WhiteOverlay UI component * feat: adjust the style and behavior of the login form * ci: update the node version * chore: remove unnecessary google image * ci: update the node version * feat: set timeout to prevent indefinite loading --------- Co-authored-by: kotaaaa <kota.k.1132.pda@gmail.com> * Feature: Delete container UI in group single page (#306) * feat: implement displaying dialog * docs: add docs to function and interface * chore: adjust the documentation * docs: typo * docs: update the content * docs: update doc * docs: add doc where missing * docs: add missing symbol for doc * docs: fix the explanation * docs: fix the explanation * Fix: Highlight the bottom icons on sub pages (#302) * Chore: Add pre-push hook to run test (#301) * ci: add pre-push hook to run test * Revert "ci: add pre-push hook to run test" This reverts commit d01a418. * ci: add pre-push hook to run test * Docs: Improve Community Standards (#307) * docs: create SECURITY.md * docs: create `CODE_OF_CONDUCT.md` * Feat : Get users name in profile page (#308) * feat: implement getCurrentUserId api client * test: test getCurrentUser api client * fix: fix api client to resolve with the issue #255 * refactor: refactor the TSDoc comment in authApiClient * feat: implement getUserById API client * feat: apply api clients to Profile page * refactor: remove unnecessary temp solution * refactor: update the comments in api client * test: test getUserByUserId function * refactor: add comments * refactor: update TSDoc comments * refactor: add issue reference in Profile page * fix: return => returns in getUserName TSDoc * fix: duplicate import line in userApiClient.server * fix: relative import path to absolute * refactor: add better comment to getUserName function * refactor: add TSDoc comment to UserProfile prop interface * fix: remove return types from TSDoc * refactor: add line break before `it` * refactor: use template literal * refactor: use template literal * refactor: update TSDoc and add error handling to throw error * fix: remove relative path * fix: remove relative path * test: refactor test to make it better --------- Co-authored-by: ShoeheyOt <142355969+ShoeheyOt@users.noreply.github.com> Co-authored-by: kanta1207 <99339182+kanta1207@users.noreply.github.com> Co-authored-by: Anukrati Mehta <anukratimehta@gmail.com> Co-authored-by: kotaaaa <kota.k.1132.pda@gmail.com>

feat: implement deleteGroupConfirmDialog

9849cc3

kanta1207 changed the title ~~feat: implement deleteGroupConfirmDialog~~ Feat: implement deleteGroupConfirmDialog Feb 21, 2024

kanta1207 self-assigned this Feb 21, 2024

kanta1207 linked an issue Feb 21, 2024 that may be closed by this pull request

Implement Delete Group Confirm Dialog component #244

Closed

2 tasks

kanta1207 requested review from kotaaaa, nick-y-ito and ShoeheyOt February 21, 2024 20:40

kanta1207 marked this pull request as ready for review February 21, 2024 20:50

kanta1207 changed the title ~~Feat: implement deleteGroupConfirmDialog~~ Feat: implement DeleteGroupConfirmDialog Feb 21, 2024

nick-y-ito reviewed Feb 22, 2024

View reviewed changes

kanta1207 mentioned this pull request Feb 23, 2024

Feature: Implement food DELETE functionality #246

Merged

12 tasks

kanta1207 added 3 commits February 26, 2024 17:03

Merge branch 'develop' into feature/244-delete-group-confirm-dialog-c…

d12a2d2

…omponent

feat: re-implement delete group dialog with new file structure

b40824e

fix: out of scope, env variable name

ef85449

kanta1207 commented Feb 27, 2024

View reviewed changes

kanta1207 changed the title ~~Feat: implement DeleteGroupConfirmDialog~~ Feat: implement DeleteGroupDialog Feb 27, 2024

nick-y-ito approved these changes Feb 27, 2024

View reviewed changes

nick-y-ito requested changes Feb 27, 2024

View reviewed changes

kanta1207 added 2 commits February 26, 2024 22:06

fix: restructure to close dropdown menu on dialog close

9c5e7d2

fix: remove unnecessary asChild in DropdownMenuItem

00e3444

kanta1207 commented Feb 27, 2024

View reviewed changes

nick-y-ito reviewed Feb 27, 2024

View reviewed changes

fix: close dropdown on closing dialog

0e04270

nick-y-ito approved these changes Feb 27, 2024

View reviewed changes

kanta1207 merged commit 8b7608c into develop Feb 27, 2024
3 checks passed

kanta1207 deleted the feature/244-delete-group-confirm-dialog-component branch February 27, 2024 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: implement DeleteGroupDialog #248

Feat: implement DeleteGroupDialog #248

kanta1207 commented Feb 21, 2024 •

edited by nick-y-ito

Loading

nick-y-ito left a comment •

edited

Loading

nick-y-ito Feb 22, 2024 •

edited

Loading

kanta1207 Feb 27, 2024

nick-y-ito Feb 22, 2024 •

edited

Loading

kanta1207 Feb 27, 2024

kanta1207 Feb 27, 2024 •

edited

Loading

kanta1207 Feb 27, 2024

kanta1207 Feb 27, 2024

kanta1207 Feb 27, 2024

kanta1207 commented Feb 27, 2024

nick-y-ito left a comment

nick-y-ito Feb 27, 2024

nick-y-ito Feb 27, 2024

nick-y-ito Feb 27, 2024

nick-y-ito Feb 27, 2024

nick-y-ito left a comment •

edited

Loading

kanta1207 commented Feb 27, 2024

kanta1207 Feb 27, 2024

nick-y-ito Feb 27, 2024 •

edited

Loading

nick-y-ito left a comment

nick-y-ito Feb 27, 2024 •

edited

Loading

kanta1207 commented Feb 27, 2024

nick-y-ito left a comment

Feat: implement DeleteGroupDialog #248

Feat: implement DeleteGroupDialog #248

Conversation

kanta1207 commented Feb 21, 2024 • edited by nick-y-ito Loading

Overview

Changes

Review points

Screen Captures

Assignee Checklist:

Reviewer Checklist:

nick-y-ito left a comment • edited Loading

Choose a reason for hiding this comment

Review Points

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanta1207 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanta1207 commented Feb 27, 2024

nick-y-ito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-y-ito left a comment • edited Loading

Choose a reason for hiding this comment

kanta1207 commented Feb 27, 2024

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

nick-y-ito left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

kanta1207 commented Feb 27, 2024

nick-y-ito left a comment

Choose a reason for hiding this comment

kanta1207 commented Feb 21, 2024 •

edited by nick-y-ito

Loading

nick-y-ito left a comment •

edited

Loading

nick-y-ito Feb 22, 2024 •

edited

Loading

nick-y-ito Feb 22, 2024 •

edited

Loading

kanta1207 Feb 27, 2024 •

edited

Loading

nick-y-ito left a comment •

edited

Loading

nick-y-ito Feb 27, 2024 •

edited

Loading

nick-y-ito Feb 27, 2024 •

edited

Loading