Fix 4363 modify kanban menu #5337
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
|
@kiridarivaki This is great work for a first PR :) ! Thank you! Could you please check the view switcher? I think it had a negative impact there The rest looks good 
Would be great to tackle the "disabled" state in a separate PR yes! |
|
@FelixMalfait Thank you for noticing! Will work on it now:) |
|
@FelixMalfait sorry for bothering you with this, but locally, with my implemented changes, i can't see the problematic menu item in the view switcher:
|
|
@kiridarivaki can you try adding a view? |
|
@FelixMalfait oh sorry you meant the misplacement of the pencil icon is the issue? Now i see it, thanks! |
|
@kiridarivaki yes exactly :) |
| @@ -158,6 +158,7 @@ export const RecordIndexOptionsDropdownContent = ({ | |||
| onClick={() => handleSelectMenu('hiddenFields')} | |||
| LeftIcon={IconEyeOff} | |||
| RightIcon={IconChevronRight} | |||
| hasRightIcon={true} | |||
There was a problem hiding this comment.
why do you introduce this and not look at RightIcon directly?
There was a problem hiding this comment.
i used hasRightIcon in line 73, because it caused a weird movement when hovering over the right icon, as i show in the video
Untitled.video.-.Made.with.Clipchamp.mp4
There was a problem hiding this comment.
Thanks! what I ment was hasRightIcon seems synonymous of !isUndefinedOrNull(RightIcon) no? maybe I'm missing something. But it doesn't seem like a clean component api if you can have rightIcon=XXX and hasRightIcon=false (contradiction)
| <StyledMenuItemRightContent> | ||
| <MenuItemLeftContent LeftIcon={RightIcon ?? undefined} text={''} /> | ||
| {hasRightIcon && ( | ||
| <MenuItemLeftContent LeftIcon={RightIcon ?? undefined} /> |
There was a problem hiding this comment.
LeftIcon={RightIcon} ? Feels off :)
Let's try to maintain a clean/beautiful/understandable API for all of our components
They should be usable as a standalone library, not built for a specific concept
Thanks 🙏
There was a problem hiding this comment.
Oh yes, I wanted to ask you about it, as I saw that there is a MenuItemLeftContent but not a MenuItemRightContent. Should I create MenuItemRightContent? I though that adding a RightIcon prop to the MenuItemLeftContent isn't a good practice either. What is preferable? I'm sorry for the mediocre code quality, trying my best as I'm still learning :)
There was a problem hiding this comment.
Looking at the figma, what we externally want is a MenuItem that can optionally take a LeftIcon, a grip, a RightIcon (with hoverable behavior). This is our goal.
I actually think having this internally MenuItemLeftContent is a mistake and should not exist (I see it's already used as if it was an external component). I would say we should not introduce a MenuItemRightContent and do something similar as in packages/twenty-front/src/modules/ui/navigation/menu-item/components/MenuItemSelect.tsx to display the right Icon for now.
Bonus: If you feel confortable, I would advocate to move back the code of MenuItemLeftContent into MenuItem, and update MenuItemLeftContent usage by MenuItem usage.
There was a problem hiding this comment.
@charlesBochet check Discord :)
| @@ -14,7 +14,7 @@ type MenuItemLeftContentProps = { | |||
| className?: string; | |||
| LeftIcon: IconComponent | null | undefined; | |||
| showGrip?: boolean; | |||
| text: string; | |||
| text?: string; | |||
There was a problem hiding this comment.
What does it mean to have a MenuItem without content?
There was a problem hiding this comment.
Should i make the text equal to an empty string? I thought text was not needed when creating the right content of the menu item as it's only an icon (line 70 in MenuItem.tsx)
There was a problem hiding this comment.
see MenuItemSelect.tsx to see how to add a Right icon and implement it in MenuItem direclty. Do not MenuItemLeftContent
|
Thanks for the updates! I see there's a small conflict with main, could you also please merge the most recent version of main? Hopefully we can merge that soon after the small fixes :) |
|
@FelixMalfait @charlesBochet Thank you for the feedback! I'll try my best to do these fixes and improve :) |
Changes:
As this is my first pr, feedback is very welcome!
Note:
These changes cover most of #4363 . Should i make a separate pr for the implementation of "Disabled" state?