-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
add: objectName in fav folder #8785
base: main
Are you sure you want to change the base?
add: objectName in fav folder #8785
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.
PR Summary
This PR enhances the navigation drawer by adding object type labels next to favorite items, improving clarity and context for users.
- Implemented clip-path/opacity-based visibility for better accessibility and layout compared to display:none
- Added
objectName
display inNavigationDrawerItem
with "·" separator and lighter styling - Updated truncation logic to handle label and objectName as a single unit for better space management
- Added
StyledItemObjectName
component with specific styling for object type labels - Known limitation:
NavigationDrawerAnimatedCollapseWrapper
conflicts with flex/truncation properties when wrapping text elements
4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
onClick={(e) => { | ||
e.stopPropagation(); | ||
e.preventDefault(); | ||
}} |
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.
style: stopPropagation and preventDefault could interfere with keyboard navigation. Consider handling keyboard events separately.
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.
Some comments but it looks nice, well done
...twenty-front/src/modules/ui/navigation/navigation-drawer/components/NavigationDrawerItem.tsx
Outdated
Show resolved
Hide resolved
...twenty-front/src/modules/ui/navigation/navigation-drawer/components/NavigationDrawerItem.tsx
Outdated
Show resolved
Hide resolved
)} | ||
|
||
<NavigationDrawerAnimatedCollapseWrapper> |
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.
we need that back
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.
@martmull I looked into it. But it was not possible. As I described in the description that having NavigationDrawerAnimatedCollapseWrapper
on top of any text or div won't let the truncation property work.
2024-11-29.17-29-58.mov@harshrajeevsingh The folder's right icon (here IconDotsVertical) on hover needs background! :) |
@ehconitin But, It's working for me Screencast.from.2024-11-29.17-42-20.webm |
Closes: #8549
It was quite complex to get this right. So, I went through Notion's website to see how they implemented it.
Instead of using
display: none
or having a space reserved for the Icon, I used clip-path & opacity trick to achieve the desired behaviour. This maintains accessibility and helps in label or ObjectName to take the full space.Also, truncation now works for label & objectName as a whole instead of separately, as seen in my previous PR.
Caveats
The only problem that now remains is not having
NavigationDrawerAnimatedCollapseWrapper
. Having it on top of any text or div won't let the flex or truncation property work.Screencast.from.2024-11-28.13-37-31.webm