Improve inline previews for links and images #524

maxpoulin64 · 2016-07-23T03:05:12Z

This is a CSS mod I have had for a little while that improves the readability and space efficiency of the link and image previews, so I thought it would be time to finally share it.

For me, this makes it viable to have them all expand by default and you don't get extra huge images in your face all the time. You can see on the first screenshot I have hidden the toggle button entirely, as I don't feel like I need it anymore.

maxpoulin64 · 2016-07-24T06:26:27Z

@astorije: you asked how they looked before, there:

astorije · 2016-08-10T00:44:51Z

client/css/style.css

 }

 #chat .toggle-content.show {
-	display: inline-block !important;
+	display: block !important;


While we're at it, did you look if it was possible/a good idea to get rid of the !important here?

So I tested this and it works just with without the !important.
However, this rule is redundant with this Bootstrap one anyway so I think we should remove it.

xPaw · 2016-08-20T09:24:50Z

👍

astorije · 2016-08-31T04:53:08Z

Alright, finally found some time to review this!

It looks really nice, but I found out a few issues we'll want to fix before merging:

On non-image links (tested with GitHub URLs), picture goes out of the box.
On desktop, for non-image links, box width is fixed and does not shrink when window size gets smaller.
On mobile, for image links, box takes all width (I believe this issue is already present on master, but since you are improving the whole thing...).

maxpoulin64 · 2016-09-04T00:58:12Z

@astorije: just fixed the images. As for the links overflowing, this is unfortunately not possible because of the table layout (which is why I didn't notice it at first, it works beautifully with my non-broken chat layout. Table cells aren't supposed to contain block elements in them)

astorije · 2016-09-12T05:07:35Z

just fixed the images

Huh, really? I still have this after pulling your branch and rebasing with master (and also when pulling your branch as is with ./scripts/run_pr.sh 524):

As for the links overflowing, this is unfortunately not possible because of the table layout (which is why I didn't notice it at first, it works beautifully with my non-broken chat layout. Table cells aren't supposed to contain block elements in them)

The second issue is pretty bad as content is missing, and does not exist in master. Maybe we should get #523 reviewed and merged first then? Too bad, this current PR seemed like a no-brainer at first 😞 (#523 is a bit harder to jump in).

astorije · 2016-10-11T13:02:57Z

Marking this as do not merge until issue(s) above is fixed and #523 is merged.

astorije · 2016-12-05T05:39:41Z

@maxpoulin64, do you think we should close this in favor of #701?

AlMcKinlay · 2016-12-12T12:44:37Z

@astorije I think that would be a sensible decision if we think #701 is likely to get merged (I think it is), then this should probably be closed.

astorije · 2016-12-19T02:24:08Z

Okay, closing this for now since @MaxLeiter wants to work on #701 that supersedes it. If for some reason #701 cannot make it (but I hope it will!) then we can re-open this.

astorije · 2017-04-03T06:53:35Z

I suggest reopening this since #701 isn't gonna happen anytime soon. I can try to revive it tomorrow

astorije · 2017-04-04T05:56:43Z

So, I have revived this and while the first issue I mentioned in my earlier comment was fixed, the other 2 were not:

It's not obvious but the first preview does not show ellipsis and goes behind the user list. You can tell with the lack of margin on the right.

@maxpoulin64, I know you have been out of the game for a bit now, but since your flexbox layout got merged, I'm hoping you could help us with this pleeeease? It would be super sweet to merge this! :)
I have just checked and these 2 issues are absent from master.

Bootstrap was taking over these declarations because they use `!important`.

Also fix the preview description not respecting the ellipsis, and update the image size and margin to nicely align with text.

astorije · 2017-04-04T06:33:43Z

Alriiiight thanks to some efforts and help from @maxpoulin64, all issues are fixed, including 2 I discovered along the way, yay! 🎉

Who wants to test this? :)

astorije · 2017-04-04T06:34:59Z

client/views/toggle.tpl

@@ -1,5 +1,5 @@
 {{#toggle}}
-<div class="toggle-content">
+<div class="toggle-content toggle-type-{{type}}">


I don't think this is necessary anymore @maxpoulin64, what do you think?

Can be useful for themes.

xPaw · 2017-04-04T09:07:43Z

client/views/toggle.tpl

@@ -1,5 +1,5 @@
 {{#toggle}}
-<div class="toggle-content">
+<div class="toggle-content toggle-type-{{type}}">


Can be useful for themes.

xPaw · 2017-04-04T09:08:03Z

client/css/style.css

 }

 #chat .toggle-content .thumb {
-	max-height: 110px;
-	max-width: 210px;
+	float: left;


Tried flex instead of float?

I have not. Floats are very much designed for this kind of usage, and it works just fine like this sooo I'm tempted to call it good enough and go on with my life :) (also it was like this initially, I did not try to be creative)
All I missing a clear issue that flexbox addresses here?

bews · 2017-04-20T15:33:05Z

Site preview images are too small now, I don't even see the content now. This is how it looks:

vs. before:

Now prefetch is pointless for me, because I still need to click on every link.

astorije · 2017-04-21T06:39:12Z

Site preview images are too small now, I don't even see the content now.

There are 2 types of previews: images & documents.
What you are showing here are documents, and we are displaying one image as part of the link preview (I forgot how that image gets picked over another on a page).
That means you can share a link to a GitHub repo and the picture would be the user's avatar. You certainly don't want that avatar to take that much real estate on the chat.
Images on the other hand are very focused, so when you share a link to a picture, it's with more confidence that we can display a bigger one.

Now prefetch is pointless for me, because I still need to click on every link.

I am sure you mean well, but don't forget that this is an open source project: no one is working for money (it's 2:30am here as I type, and I spend time most nights on this), nor for you specifically. Also, don't forget that the coldness of the screen vs. live discussion can make it look like you feel entitled to a service and requests, and I have noticed people are less inclined to help in these situations.

What you expose here is a personal opinion and as such unlikely to reflect in code changes. That PR was approved and desired by multiple people, and deemed the best approach UX-wise for a largest audience.
That being said, it doesn't mean there are no solutions:

If an opinion materializes in something actually better for the user regardless of preferences (or personal abilities, i.e. accessibility concerns), then a PR is almost always welcome. Make sure to expose your thought process in an issue first if you want to make sure the team is on board with it. For example, Better embedding using embed.js #701 is severely looking for help and I'm sure @MaxLeiter would be happy to guide you.
People have opinions, especially in the IRC world, and that's why we introduced the custom CSS stylesheet in the settings. With a bit of tweaking, it is possible to make this preview bigger again.

Makes sense? 😃

bews · 2017-04-22T03:25:41Z

That means you can share a link to a GitHub repo and the picture would be the user's avatar. You certainly don't want that avatar to take that much real estate on the chat.

Definitely, but there are also other sites on the internet aside for GitHub.
This preview picture is extremly useful when target site specialize in graphic content - I deside to open link or not judging what it look like. For example, today only preview motivated me to read a few articles and news.

deemed the best approach UX-wise for a largest audience

By ruining a nice functionality? It's really simple: before this change I could use this preview to save time, now I can't.

(it's 2:30am here as I type, and I spend time most nights on this), nor for you specifically

Alright, it's 5:20 am here (don't break this combo now 😄), and it's the second night I spent improving this project. I'm not doing this just for you too 😸.
I do all improvements primarily for myself, because I really need an irc client like this. But at the same time I want to provide a better experience for everyone else. This is exactly why I'm writing this now, while my local copy is already fixed.

If an opinion materializes in something actually better for the user regardless of preferences

And how do we decide is it better or not? By making a poll?

Anyway I plan to redesign this previews (a bit more in irccloud style) and I almost sure it will work well with #701 in future (no, I have no intention to help with it, sorry).
I'll upload it to my fork in case there will be people who don't like this change too (ATM most of them believe it's just a bug, since it's not in the release yet).

MaxLeiter · 2017-04-22T18:48:57Z

@bews if you have a fix (or when you do), feel free to open a PR for discussion from other maintainers and users. @astorije meant this PR was merged with a fair amount of support, but that doesn't mean it's perfect. IIRC from IRC you have a large screen - maybe no one's tested on that yet and that's where all this confusion is coming from.

And I disagree that most people believe it's a bug - otherwise there would be an open issue for it.

Improve inline previews for links and images

maxpoulin64 added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. second review needed labels Jul 23, 2016

astorije self-assigned this Jul 23, 2016

astorije reviewed Aug 10, 2016
View reviewed changes

maxpoulin64 force-pushed the PR/inline-preview branch from a7a0d64 to e4ddc1b Compare September 4, 2016 00:52

astorije added the Meta: Do Not Merge This PR should not be merged. label Sep 18, 2016

xPaw removed the Meta: Do Not Merge This PR should not be merged. label Oct 1, 2016

astorije added the Meta: Do Not Merge This PR should not be merged. label Oct 11, 2016

xPaw removed the second review needed label Dec 17, 2016

astorije closed this Dec 19, 2016

astorije deleted the PR/inline-preview branch February 14, 2017 04:44

astorije restored the PR/inline-preview branch April 4, 2017 05:39

astorije reopened this Apr 4, 2017

Improve inline previews for links and images

6a273d8

astorije force-pushed the PR/inline-preview branch from e4ddc1b to 6a273d8 Compare April 4, 2017 05:41

astorije added 2 commits April 4, 2017 02:09

Fix image preview talking full width

d842517

Bootstrap was taking over these declarations because they use `!important`.

Fix link preview title going underneath the user list

dce42df

Also fix the preview description not respecting the ellipsis, and update the image size and margin to nicely align with text.

astorije force-pushed the PR/inline-preview branch from 649c34e to dce42df Compare April 4, 2017 06:31

astorije removed the Meta: Do Not Merge This PR should not be merged. label Apr 4, 2017

astorije added this to the 2.3.0 milestone Apr 4, 2017

astorije removed their assignment Apr 4, 2017

astorije approved these changes Apr 4, 2017

View reviewed changes

xPaw reviewed Apr 4, 2017

View reviewed changes

xPaw approved these changes Apr 6, 2017

View reviewed changes

astorije merged commit b8c4946 into master Apr 6, 2017

astorije deleted the PR/inline-preview branch April 6, 2017 07:01

astorije mentioned this pull request May 3, 2017

Better previews #1130

Closed

3 tasks

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#524 from thelounge/PR/inline-preview

6b75bf8

Improve inline previews for links and images

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve inline previews for links and images #524

Improve inline previews for links and images #524

maxpoulin64 commented Jul 23, 2016

maxpoulin64 commented Jul 24, 2016

astorije Aug 10, 2016

astorije Aug 31, 2016 •

edited

Loading

xPaw commented Aug 20, 2016

astorije commented Aug 31, 2016 •

edited

Loading

maxpoulin64 commented Sep 4, 2016

astorije commented Sep 12, 2016

astorije commented Oct 11, 2016

astorije commented Dec 5, 2016

AlMcKinlay commented Dec 12, 2016

astorije commented Dec 19, 2016

astorije commented Apr 3, 2017

astorije commented Apr 4, 2017

astorije commented Apr 4, 2017

astorije Apr 4, 2017

xPaw Apr 4, 2017

astorije Apr 5, 2017

xPaw Apr 4, 2017

xPaw Apr 4, 2017

astorije Apr 5, 2017

bews commented Apr 20, 2017 •

edited

Loading

astorije commented Apr 21, 2017

bews commented Apr 22, 2017

MaxLeiter commented Apr 22, 2017 •

edited

Loading

Improve inline previews for links and images #524

Improve inline previews for links and images #524

Conversation

maxpoulin64 commented Jul 23, 2016

maxpoulin64 commented Jul 24, 2016

astorije Aug 10, 2016

Choose a reason for hiding this comment

astorije Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

xPaw commented Aug 20, 2016

astorije commented Aug 31, 2016 • edited Loading

maxpoulin64 commented Sep 4, 2016

astorije commented Sep 12, 2016

astorije commented Oct 11, 2016

astorije commented Dec 5, 2016

AlMcKinlay commented Dec 12, 2016

astorije commented Dec 19, 2016

astorije commented Apr 3, 2017

astorije commented Apr 4, 2017

astorije commented Apr 4, 2017

astorije Apr 4, 2017

Choose a reason for hiding this comment

xPaw Apr 4, 2017

Choose a reason for hiding this comment

astorije Apr 5, 2017

Choose a reason for hiding this comment

xPaw Apr 4, 2017

Choose a reason for hiding this comment

xPaw Apr 4, 2017

Choose a reason for hiding this comment

astorije Apr 5, 2017

Choose a reason for hiding this comment

bews commented Apr 20, 2017 • edited Loading

astorije commented Apr 21, 2017

bews commented Apr 22, 2017

MaxLeiter commented Apr 22, 2017 • edited Loading

astorije Aug 31, 2016 •

edited

Loading

astorije commented Aug 31, 2016 •

edited

Loading

bews commented Apr 20, 2017 •

edited

Loading

MaxLeiter commented Apr 22, 2017 •

edited

Loading