-
-
Notifications
You must be signed in to change notification settings - Fork 385
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
ui(web): Fixed overflow issues for very long titles #152
ui(web): Fixed overflow issues for very long titles #152
Conversation
…ellipsis ui(web): Very long titles now break in edit mode instead of overflowing horizontally
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Thanks a lot for taking the time and fixing that! The PR looks good to me, I've left a couple of suggestions related to that fix, if you don't have time for them, it's ok, I can do them :)
@@ -96,7 +96,7 @@ function ViewMode({ | |||
}: Props) { | |||
return ( | |||
<Tooltip delayDuration={500}> | |||
<div className="flex items-center gap-3 text-center"> | |||
<div className="flex max-w-full items-center gap-3 text-center"> |
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.
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.
Ah, good catch--I didn't even notice the tooltip there! I ended up using break-words
to ensure that more typical titles with multiple words don't break in strange places. The editable title field still uses break-all
, since where the lines break doesn't matter so much there, as long as the whole title is visible!
@@ -65,7 +65,11 @@ function ListView({ | |||
</div> | |||
<div className="flex h-full flex-1 flex-col justify-between gap-2 overflow-hidden"> | |||
<div className="flex flex-col gap-2 overflow-hidden"> | |||
{title && <div className="flex-none shrink-0 text-lg">{title}</div>} | |||
{title && ( | |||
<div className="flex-none shrink-0 overflow-hidden text-ellipsis text-lg"> |
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.
To fix the overflow once and for all, it seems that we need a line-clamp-2
here as well. I have it for links in https://github.com/MohamedBassem/hoarder-app/blob/9b5ef3a70e0e7300124076939587a038b10cb577/apps/web/components/dashboard/bookmarks/LinkCard.tsx#L20 but seems like I forgot to add it to the other types. Let's remove it from LinkCard and add it here?
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.
Done, thanks!
@@ -104,7 +108,11 @@ function GridView({ | |||
{img && <div className="h-56 w-full shrink-0 overflow-hidden">{img}</div>} | |||
<div className="flex h-full flex-col justify-between gap-2 overflow-hidden p-2"> | |||
<div className="grow-1 flex flex-col gap-2 overflow-hidden"> | |||
{title && <div className="flex-none shrink-0 text-lg">{title}</div>} | |||
{title && ( | |||
<div className="flex-none shrink-0 overflow-hidden text-ellipsis text-lg"> |
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.
Same here about line-clamp-2
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.
🫡
editClassName="p-2 text-center text-lg" | ||
viewClassName="line-clamp-2 text-lg" | ||
editClassName="p-2 text-center text-lg break-all" | ||
viewClassName="inline line-clamp-2 text-lg text-ellipsis" |
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.
Don't you think that a break-all
would be ok here? specially that we have the line-clamp-2
already?
You're the designer though, so you know what's best UX wise :D
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.
I ended up going with a combo of break-all
for the editable field and break-words
for the view field, as mentioned above. It seems to work fine with all titles that I've tested on both desktop and mobile, but let me know if you notice any edge cases that don't work properly! (I also took out the text centering, since multi-line titles that truncate with an ellipsis look pretty strange to my eye… just my personal preference, but easy enough to put back if it's not to your liking!)
…e with an ellipsis ui(web): Tooltips for long titles now wrap to multiple lines as needed ui(web): Aligned titles in preview panes to the left margin
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.
This is perfect, thank you so much! And thanks again for addressing the comments!
Some very long titles, particularly assets with hashed filenames, will overflow the given space in certain views. The problem is most noticeable when accessing the preview modal on mobile, where it pushes the edit button offscreen and forces the user to scroll horizontally:
This tweak terminates all overly long titles with an ellipsis, caps the editable title field at a viewable length, and forces long titles to wrap in edit mode to prevent horizontal overflow.