An image viewer popup for thumbnails and image previews #1325

astorije · 2017-07-12T07:08:29Z

Inspired from #1130 for the styling, with some improved logic like support for Escape key.

TODOs

Split thumbnail and content into 2 adjacent links to make sure clicking on thumbnail margin opens the viewer (will also make code simpler)
NB: Not sure why, but when I switched to this it fixed the image preview shrinking on master 🎉
Try to fix link going beyond image borders when resizing
Check / Make GPU taking care of animation

astorije · 2017-07-12T07:12:38Z

The flexbox thing behind link/image on the viewer is kinda 💩. Play with resize, or wide images, and the button won't stick to the image anymore, and the link will be on the image + on big top/bottom margins.
Probably not the right way to go, but I have made an unreasonable amount of attempts, so I'd love some help :D (or maybe it's good enough for this PR and can be improved later, I'd vote for that!).

Reason why 2 links btw is because I don't want the black areas around the button to be clickable too. Only image and button are/should be clickable.

MaxLeiter · 2017-07-12T07:20:17Z

Personally not sure how I feel about the open image button -- I think most people can figure out save as or drag to desktop. Overall great feature.

MiniDigger · 2017-07-12T07:47:38Z

👍 on this one, I am currently using the one from bews PR and it works great.

what limits the size of the image? can't we make it bigger? I also don't like how the button looks, but I can just change that via css. I definitely am in favor of having such a button since you might want to get to the actual site where that image is coming from (eg. viewing youtube thumbnail and then watching the video)

xPaw · 2017-07-12T07:56:18Z

client/js/renderPreview.js

+
+// Reason to make this a specific handler is, for images, to open the image
+// viewer even when clicking in the margins of the thumbnail
+$("#viewport").on("click", ".toggle-content.toggle-type-image", function() {


Have you tried stopPropagation in this event? It might be possible to get rid of the event above.

Okay that won't work as you want the thumbnail too.

I'll rewrite that part to make it simpler and remove an edge case (accidentally clicking on margin of image in link previews, not image previews, will open the link instead of the viewer).

xPaw · 2017-07-12T07:57:03Z

client/js/renderPreview.js

+	return false;
+});
+
+$(document).on("click", "#image-viewer", function() {


You can cache #viewport into a variable and bind all these events on it instead of document.

You can cache #viewport into a variable

Will do!

bind all these events on it instead of document.

Any benefits? By not binding on body, it means your focus can be off and not catch the event, no?

xPaw · 2017-07-12T07:57:41Z

client/js/renderPreview.js

+
+$(document).keydown(function(e) {
+	switch (e.keyCode ? e.keyCode : e.which) {
+	case 27: // Escape


We also do this dropdown menus, might be a good idea to combine or something.

I don't really want to get into keyboard handling as part of this PR. Can we do it later?
(Also, I'm not 100% sure we should, hesitating a bit, let's postpone this I'd say)

AlMcKinlay · 2017-07-12T11:44:41Z

Personally not sure how I feel about the open image button -- I think most people can figure out save as or drag to desktop. Overall great feature.

I disagree, let's not make assumptions on that sort of level, not everyone knows these things. I would however suggest that maybe instead of this button, we could have a button that has this icon which opens it in a new tab. But that's just a small thing.

I'll do a proper review of this soon, looks good in general, @astorije

xPaw · 2017-07-12T12:46:32Z

I don't think using blur (especially with transition) is worth the performance hit considering how dark the container already is.

For some reason object-fit: contain makes the link hitbox square, so hitting empty space still opens the link (try https://i.redd.it/bk48m38ioz8z.jpg)

MaxLeiter · 2017-07-12T15:29:59Z

A lot of the time instead of using blur people create a div with a slight alpha like here:

https://codepen.io/imprakash/pen/GgNMXO

Could that be a better solution, @xPaw?

astorije · 2017-07-12T15:31:53Z

Personally not sure how I feel about the open image button -- I think most people can figure out save as or drag to desktop.

Additionally to what @YaManicKill said, it's less trivial on mobile to open context menu than to click on a button, and while it's convenient it's not obvious that image is clickable.
For the record, Google Images shows this kind of button too.
Also, when #1307 is merged, button/link will open the original image instead of the proxied picture, which is usually what you want to do when saving or zooming (at this point we won't be compressing/resizing the image, but user does not know that).

what limits the size of the image? can't we make it bigger?

Image is: fluid until 100% of the image width max. So if image is 1000px width and your screen is 2000px wide, image will take half of it. If your screen is 500px wide, image will be reduced to fit 100% of the screen.
Does that make sense? I feel like it doesn't :D
To see it in full-size, you can click the image or the button.

For some reason object-fit: contain makes the link hitbox square, so hitting empty space still opens the link (try https://i.redd.it/bk48m38ioz8z.jpg)

Yeah, that's the issue I was mentioning above. I think I found a way to fix this, I'll try this and see what happens.

I don't think using blur (especially with transition) is worth the performance hit considering how dark the container already is.

You don't like nice things :D
I honestly think there is no issue with this at all. I will try with the slowest devices of our generation and our parent's (i.e. current phones in my household) but I doubt this will have any noticeable impact. Especially since there is no scroll or else on the blurred content.
If it's an issue, I will use the transform: translate3d(0,0,0); or transform: translateZ(0); trick, but I don't even think we'll need this.
And yeah, of course it's worth it, it's pretty! :)

astorije · 2017-07-12T15:33:14Z

A lot of the time instead of using blur people create a div with a slight alpha like here:

If I understand correctly, this is doing both. It looks pretty slick as is!

astorije · 2017-07-15T17:23:15Z

I fixed both issues mentioned in the PR description so this should be good for review. The max-height for image is pretty disgusting but for the life of me I couldn't find anything else that works (object-fit: contain is out of the picture due to how it works).
Any suggestions? Or you're cool with the current solution?

@xPaw, I couldn't find where to check that GPU is being leverage, could you advise?

AlMcKinlay

Looks good, pretty simple. I think I'd like the icon to be as I described in my earlier comment, but it's not a blocker. Nice feature.

AlMcKinlay · 2017-07-15T20:11:20Z

client/js/renderPreview.js

+
+/* Image viewer */
+
+// FIXME Remove #input focus when this is open


If you aren't going to fix this in this PR (which I'm ok with) then stick up an issue on github for it and put in the number here. Otherwise this will get missed. I hate random "FIXME" and "TODO" s that don't have any number attached to them

Good idea, here it is: #1342

xPaw · 2017-07-16T19:23:12Z

client/css/style.css

+	opacity: 1;
+}
+
+#image-viewer .image-link img {


Wouldn't actual padding on here work instead of using calc?

Actually, left/right padding calc was unnecessary as the link itself has a margin (I removed them), but for the max height, padding doesn't help. What this means is that the image's max height must be all screen minus its margins and the button height, and I can't get to anything else that would work and be less "hardcoded"... :/

xPaw · 2017-07-16T19:24:45Z

client/js/renderPreview.js

+	return false;
+});
+
+$(document).on("click", "#image-viewer", function() {


Pretty sure the way you're binding the event here does not help and $("#image-viewer").on("click") should work exactly the same.

Good call, done.

xPaw · 2017-07-16T19:25:45Z

client/js/renderPreview.js

+		type: link.parent().hasClass("toggle-type-image") ? "image" : "link"
+	}));
+
+	$("body").addClass("image-viewer-opened");


Use $(document.body)

Thanks, done.

xPaw · 2017-07-16T19:26:22Z

client/js/renderPreview.js

+	$("body").addClass("image-viewer-opened");
+}
+
+function closeImageViewer() {


Clearing out image-viewer in here might be a good idea to unload it from memory.

Agreed, done (after transition has ended).

astorije · 2017-07-16T22:17:19Z

I believe I have addressed all comments.

xPaw

@astorije, merge whenever.

An image viewer popup for thumbnails and image previews

astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jul 12, 2017

astorije added this to the 2.4.0 milestone Jul 12, 2017

xPaw reviewed Jul 12, 2017

View reviewed changes

Fix some button styling

a010973

astorije force-pushed the astorije/image-viewer branch 2 times, most recently from e170783 to 1602c28 Compare July 15, 2017 17:15

astorije force-pushed the astorije/image-viewer branch from 1602c28 to 89d949b Compare July 15, 2017 17:23

AlMcKinlay approved these changes Jul 15, 2017

View reviewed changes

xPaw reviewed Jul 16, 2017

View reviewed changes

astorije mentioned this pull request Jul 16, 2017

Un-focus input message when image viewer is opened #1342

Closed

astorije force-pushed the astorije/image-viewer branch 2 times, most recently from ae3064f to b19b799 Compare July 16, 2017 22:14

Add an image viewer for thumbnails and image previews

511b173

astorije force-pushed the astorije/image-viewer branch from b19b799 to 511b173 Compare July 16, 2017 23:25

xPaw approved these changes Jul 17, 2017

View reviewed changes

astorije merged commit ce0e460 into master Jul 18, 2017

astorije deleted the astorije/image-viewer branch July 18, 2017 04:09

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

Merge pull request thelounge#1325 from thelounge/astorije/image-viewer

bfb4328

An image viewer popup for thumbnails and image previews

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

An image viewer popup for thumbnails and image previews #1325

An image viewer popup for thumbnails and image previews #1325

astorije commented Jul 12, 2017 •

edited by xPaw

Loading

astorije commented Jul 12, 2017

MaxLeiter commented Jul 12, 2017

MiniDigger commented Jul 12, 2017

xPaw Jul 12, 2017

xPaw Jul 12, 2017

astorije Jul 12, 2017

xPaw Jul 12, 2017

astorije Jul 12, 2017

xPaw Jul 12, 2017

astorije Jul 12, 2017

AlMcKinlay commented Jul 12, 2017

xPaw commented Jul 12, 2017 •

edited

Loading

MaxLeiter commented Jul 12, 2017

astorije commented Jul 12, 2017

astorije commented Jul 12, 2017

astorije commented Jul 15, 2017

AlMcKinlay left a comment

AlMcKinlay Jul 15, 2017

astorije Jul 16, 2017

xPaw Jul 16, 2017

astorije Jul 16, 2017 •

edited

Loading

xPaw Jul 16, 2017

astorije Jul 16, 2017

xPaw Jul 16, 2017

astorije Jul 16, 2017

xPaw Jul 16, 2017

astorije Jul 16, 2017

astorije commented Jul 16, 2017

xPaw left a comment


		/* Image viewer */

		// FIXME Remove #input focus when this is open

An image viewer popup for thumbnails and image previews #1325

An image viewer popup for thumbnails and image previews #1325

Conversation

astorije commented Jul 12, 2017 • edited by xPaw Loading

TODOs

astorije commented Jul 12, 2017

MaxLeiter commented Jul 12, 2017

MiniDigger commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlMcKinlay commented Jul 12, 2017

xPaw commented Jul 12, 2017 • edited Loading

MaxLeiter commented Jul 12, 2017

astorije commented Jul 12, 2017

astorije commented Jul 12, 2017

astorije commented Jul 15, 2017

AlMcKinlay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Jul 16, 2017

xPaw left a comment

Choose a reason for hiding this comment

astorije commented Jul 12, 2017 •

edited by xPaw

Loading

xPaw commented Jul 12, 2017 •

edited

Loading

astorije Jul 16, 2017 •

edited

Loading