Skip to content
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

Merged
merged 3 commits into from
May 16, 2024

Conversation

lilacpixel
Copy link
Contributor

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:

IMG_1670

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.

…ellipsis

ui(web): Very long titles now break in edit mode instead of overflowing horizontally
Copy link

vercel bot commented May 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hoarder-app-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2024 5:28pm
hoarder-app-landing ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2024 5:28pm

Copy link
Collaborator

@MohamedBassem MohamedBassem left a 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">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add break-all to the div of the tooltip text as well (on line 122)? The text currently overflows the tooltip.

Screenshot 2024-05-15 at 6 42 54 AM

Copy link
Contributor Author

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">
Copy link
Collaborator

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?

Copy link
Contributor Author

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">
Copy link
Collaborator

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

Copy link
Contributor Author

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"
Copy link
Collaborator

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

Copy link
Contributor Author

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
Copy link
Collaborator

@MohamedBassem MohamedBassem left a 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!

@MohamedBassem MohamedBassem merged commit 747efa5 into hoarder-app:main May 16, 2024
6 checks passed
@lilacpixel lilacpixel deleted the text-overflow-ellipsis branch May 16, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants