-
Notifications
You must be signed in to change notification settings - Fork 4
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: restructure component layers for kebab button on groupcard #389
Refactor: restructure component layers for kebab button on groupcard #389
Conversation
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.
@ShoeheyOt
Thank you for your work and for clarifying the structure with Mermaid. And, I am sorry for my late review.
I suppose some tiny adjustments might be needed. Please take a look at the comments.
containerCount: number; | ||
/** | ||
* a number of user who belongs to a group | ||
*/ |
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 ID (identifier) of ...
- The number of ...
}: IGroupCardDropdownMenuContentProps) => { | ||
return ( | ||
<DropdownMenuContent> | ||
<DropdownMenuItem> |
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.
asChild
should be added so we can remove the unnecessary div and manipulate it only with the keyboard.
Do you mind fixing the ones on the profile page as well?
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.
@nick-y-ito Thank you for your review! as we dicussed in person, I leave fixing ones on the profile page and make an another issue.
const [isDropdownMenuOpen, setIsDropdownMenuOpen] = useState(false); | ||
const [isDeleteGroupDialogOpen, setIsDeleteGroupDialogOpen] = useState(false); | ||
|
||
const kebabRef = useAutoFocus<HTMLButtonElement>(!isDeleteGroupDialogOpen && !isRenameFormOpen); |
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.
Thank you for adding this!
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.
@ShoeheyOt
Thank you for updating! LGTM :)
Overview
This PR is for refactoring, changing structure
Changes
The below 2 diagrams are comparison of how I changed component structure.
Before
After
Screen Captures
screen-capture.webm
Assignee Checklist:
Reviewer Checklist: