Add simple lightbox feature to markdown images

What does this MR do?

• Open full-screen view of image in a lightbox when clicked

Why was this MR needed?

• Improve the UX of viewing images in MRs and Issues

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

• Changelog entry added
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Closes #28387 (closed)

@dimitrieh here is WIP of the lightbox example. Test it out and lmk what you think

@samrose3 i tried it out, but it doesn't work? It should work for images within conversations (in all places where that's possible.. issues,mr's,commits,diffs?) correct?

also i liked the less dark background in the issue description mockup:

added 1 commit

• 4f4ca244 - Add simple lightbox feature to markdown images

Compare with previous version

@dimitrieh I made some updates. It should work in every MR, Issue, Note, and previewed markdown.

Changes I made:

• lightened the background
• added some animation
• added a link to the to original image in a new tab
• added a close icon in the top right corner
• dismiss on scroll

added 1 commit

• 5374f149 - Add simple lightbox feature to markdown images

Compare with previous version

marked the task All builds are passing as completed

Awesome feature!

Can we center the image on the screen? You can really tell here that there is more space on top than bottom:

I also noticed that the modal does not work for new comments. You have to refresh the page for the modal to work.

added 1 commit

• 6e9c636e - Add simple lightbox feature to markdown images

Compare with previous version

@tauriedavis thanks for catching those issues! I've fixed the centering and made it work with new comments 👍 give it another test if you don't mind 💯

marked the task All builds are passing as incomplete

@tauriedavis @samrose3 works like a champ!

As for improvements:

http://materializecss.com/media.html handles images with a sort of continuous flow. This means that the images pop out of their place, where they are at in the page. This way the animation always makes sense and gives actual context!

I also like their usage of the custom pointers.. so a "zoom in" cursor when on hover over the image

and if the image is in the lightbox we see a "zoom out" cursor

Add to this that clicking on the image actually closes the lightbox, makes for very efficient scanning of multiple images on the page. I think that opening the image in an actual tab is secondary to that UX pattern and should perhaps be done by having a popout icon next to the close icon in the upper right corner:

perhaps you can add it with http://fontawesome.io/icon/external-link-square/ and people can always just right click it to open in another tab if they don't immediately see the button 😄

added 1584 commits

• 6e9c636e...cc64eda9 - 1583 commits from branch master
• e280e3ee - Add simple lightbox feature to markdown images

Compare with previous version

Here is a codepen of the lightbox concept that would be cool for GitLab. Downside is it requires an external js library, Velocity.js, to handle the fancy animation.

@filipa Wdyt?

My current solution works, but is pretty simple.

Modalboxes can be tricky, and usually have a lot of bugs. Although it can mean adding a new lib, my suggestion is to use one that is already tested and that we are sure it won't cause us a lot of problems.

We already use a modalbox in some cases. Is it possible to do this with the same modalbox without using a new lib or creating ourselves a new lib?

From what I can understand from the issue, we just need to open an image in "fullscreen" mode and add an icon to allow the user to open it in a new tab.

I suggest do the boring first, let's see if we can make this happen with bootstrap's modalbox that we already use and adjust the CSS for this case. Let's do a POC with this, and if it doesn't work we can access the problem again, what do you think?

/cc @iamphill

@samrose3 what is the status with this 😺 ?

removed milestone

@dimitrieh had to shift focus for a bit. I'll jump back to this later this week and see if I can revise accordingly before the weekend 👍

added 2219 commits

• e280e3ee...d7a52716 - 2217 commits from branch master
• 2113a506 - Merge branch '28387-lightbox-for-quick-image-zoom' of https://gitlab.com/gitlab-…
• abc67e0f - Lightbox using Bootstrap modal

Compare with previous version

added 1 commit

• 3f35bf00 - Lightbox using Bootstrap modal

Compare with previous version

@dimitrieh I've updated this to use the Bootstrap modal for all animations and styles. It essentially grabs images with the .no-attachment-icon class and applies the modal toggle to them. Check it out an lmk what you think! See @filipa's comments as to why we are using the bootstrap modal

added 189 commits

• 3f35bf00...bbd83376 - 187 commits from branch master
• ce53b962 - Add simple lightbox feature to markdown images
• bcfc4523 - Lightbox using Bootstrap modal

Compare with previous version

@dimitrieh just pinging you on this for feedback. We can nix the idea and close the issue if the UX isn't quite as expected. Just don't want to let this issue grow old 👴

assigned to @dimitrieh

@samrose3 Are you seeing syntax errors on this branch? Tried to check it out locally but its not working for me.

assigned to @samrose3

@tauriedavis sorry about that! Needed to updated with the latest master. All rebased and back to working condition. Give it another shot 👍 :

assigned to @tauriedavis

@samrose3 Did you push? Don't see it 🤔

assigned to @samrose3

added 1514 commits

• bcfc4523...9ada13d3 - 1512 commits from branch master
• e40a08c3 - Add simple lightbox feature to markdown images
• 4f6d86f9 - Lightbox using Bootstrap modal

Compare with previous version

@tauriedavis hmm... must have been pushed while GitLab was down ??? I pushed the code then assigned it to you, but the commit showed up after all of our comments. Anyway, back over to you 👍

assigned to @tauriedavis

unmarked as a Work In Progress

hmm I'm still seeing problems when I pull and the widget says there are conflicts. Can you check again? 😓

assigned to @samrose3

added 54 commits

• 4f6d86f9...ca1e3823 - 52 commits from branch master
• 3f07aacb - Add simple lightbox feature to markdown images
• f11b6d59 - Lightbox using Bootstrap modal

Compare with previous version

@tauriedavis sorry again, new code keeps conflicting with it. Hopefully should work now 👍 🤞

assigned to @tauriedavis

Woo! 🎉 Couple notes:

Clicking on an image from your profile just scrolls to the top of the page

Can we center images on the screen?

Images in issue descriptions pop out in a new window still

I don't see the animation that @dimitrieh was suggesting above implemented. Are we going to use that? http://materializecss.com/media.html

@tauriedavis excellent comments! Thank you for the fast feedback! And per @filipa's comments, we opted to try using a modified version of the Bootstrap Modal for code reuse. However, I did implement a prototype what @dimitrieh suggested here

Personally, I don't think using the Bootstrap modal saves us much, and the animation Dimitrieh proposed is pretty unique and a nice user experience.

The prototype in that codepen closes the image on scroll but I dont see that on your branch. Should I? Here is what I am seeing:

I agree that the animation Dimitrie proposed is a nice experience

@tauriedavis I haven't implemented what is in the CodePen into GitLab 😅 I created that and before trying out the bootstrap modal. If we agree that it is the way to go, I can revise this MR to use that animation. Wdyt @filipa?

Oh gotcha 👍

assigned to @samrose3

@samrose3 catching up on older todo's. Is this still going in, or has there been discussed somewhere that we should hold off?

@dimitrieh We are in a weird situation where UX wants the clean zoom animation, but Frontend is pushing back because it would add a good chunk of code to the codebase. I'm not really sure where to go next with this one 😓

Currently, we have

• a prototype fancy animation version that has not been added to GitLab
• a simple modal version that uses the Bootstrap modals to animate and display (not fancy, but less code)

Why does frontend feel having less code in this circumstance is more important than the experience @samrose3? cc @filipa

Are there any thoughts on how we can achieve something similar with less code?

@jschatz1 @iamphill @timzallmann want to chime in on this one?

If it improves our UX, then we should consider it 👍

let's wait for @jschatz1 to make a decision here :)

I dont see the need for a fancy animation, if it works and you can click left/right and use left/right arrow keys to navigate through the images. If you could zoom the full screen image, THAT would be cool, but just an animation probably just slows the time down it takes to get me to the image I want in full screen.

mentioned in issue #33668 (moved)

@tauriedavis @iamphill @dimitrieh @tobias.s.keller. The only reason to do a fancy animation is if it matches the flow/animations of GitLab in general. The amount of code doesn't matter as much as it working well with everything else. What I don't want to do is have inconsistent animations everywhere. cc @sarrahvesselov

Thanks for bringing me into the conversation @jschatz1.

The amount of code doesn't matter as much as it working well with everything else. What I don't want to do is have inconsistent animations everywhere.

Yes, exactly. From https://gitlab.com/gitlab-org/gitlab-ce/issues/24831, it looks to me as if we are still defining/implementing our animation style. I personally love the animation @samrose3 put together. Looking at the issue I referenced and the existing UX guides for animations, does it fit into the direction we want to take GitLab' visual style?

added UX label

What we are talking about is not strictly the animation - it is the experience.

Do we want to include dismiss on scroll? This would allow the user to continue reading the flow of comments simply by scrolling down. Another question is: Is this expected behavior?

The pop in/out from where the image is on the content allows the user to easily see where they were when they dismiss the image. Perhaps this isn't a strict necessity for this version but it is an improvement.

Animation helps the experience, it is not always just about visual style.

I would suggest not to hide on scroll. two reasons:

1. Some computer mice scroll when you move them - dismissing the modal would be really ugly behaviour then. I think pressing [ESC] makes more sense
2. If we'd later add a zoom in the picture by scrolling this would not be possible anymore.

if you navigate through the pictures by next/prev buttons on the keyboard, the animation should not happen.

mit freundlichen Grüßen Tobias Keller

unassigning sam ;)

removed assignee

Perhaps a short ux meeting on this issue would be nice.. will put this in the agenda for next week.

Sounds good, thanks @dimitrieh! Would love to get this finalized and implemented 😻

As discussed per the UX meeting:

• For Mobile viewports: turn it off
• Have a button on the lightbox to get to image in new tab

If FE decides we can do a two way implementation:

1. Mvp lightbox
2. Fluent transition

Example how how we want it to be: http://materializecss.com/media.html

• Have a button on the lightbox to get to image in new tab

Some lightboxes make the whole image a link to the raw file

Edit: That isn't compatible with going to the next/previous image by clicking on the current one, which is something we may want to do in the future.

@cperessini The example also closes the image on click. I like that you don't have to find the edge of the popup or a close icon to dismiss it. But then we do lose the ability to expand to full image on click or navigate to the next.

i think close image is a function that is used more than open raw.

That isn't compatible with going to the next/previous image by clicking on the current one, which is something we may want to do in the future.

we could perhaps work with "image-zones": left part of image=previous, right=next, center=open raw, outside of lightbox=close

I like it when you can click anywhere in the background to close a lightbox. That's usually coupled with an x on the top-right corner though.

we could perhaps work with "image-zones": left part of image=previous, right=next, center=open raw, outside of lightbox=close

Something like that may work, although I think I usually expect the left 20% of an image to take me to the previous image and the remaining 80% to take me to the next one. Having a third behavior in the middle of the two could be confusing/frustrating, but we can definitely try it out in a prototype.