Feature/img modal

[xvandish]

Description of Change

This PR adds an image modal/dialog that allows images to be clicked on and expanded within the modal. Style and functionality is similar to the NYT image expansion modal like the one here. I got permission from the internal team for use of the svgs for expand/collapse since copied straight from the NYT image modal.

Features:

1. adds overlay to image that shows expandIcon on img hover as a visible indication that the image can be expanded
2. adds cursor to images on hover, for the same reason as above
3. img overlay and modal html are added during server side processing so that they're easier to change together
4. scrolling up or down on the page while the modal is open closes the modal
5. svg expand/collapse icons minfied with svgo

Here's a preview

Related Issue

#268

Motivation and Context

At the moment, all images are constrained to a max-width of ~560px. This makes some high-res images difficult to read. Allowing an image modal makes some images easer to see.

Checklist

• Ran npm run lint and updated code style accordingly
• npm run test passes
• PR has a description and all contributors/stakeholder are noted/cc'ed
• tests are updated and/or added to cover new code
• relevant documentation is changed and/or added

I'll clean up the commit history once any issues have been worked through.

[afischer]

Hey @xvandish,

Thanks a ton for the contribution! This is a fantastic addition.

I appreciate the care you put in in for conditionally adding the html for the modal only if there's an image tag present, but I think we can probably include the modal HTML in the ejs for the article page by default and ditch the ssrComponent folder. Having it always in the HTML will simplify things quite a bit.

[xvandish]

Hey @xvandish,

Thanks a ton for the contribution! This is a fantastic addition.

I appreciate the care you put in in for conditionally adding the html for the modal only if there's an image tag present, but I think we can probably include the modal HTML in the ejs for the article page by default and ditch the ssrComponent folder. Having it always in the HTML will simplify things quite a bit.

very welcome!
Sounds good! That's actually how I initially had it, I just got ahead of myself on customization. I agree its less complex inline, and probably better to leave it that way until theres a real need for more complexity. I'll make the switch back!

[xvandish]

@afischer - making the changes rn, this is the current structure

1. Image modal for expansion + svg moved to client-side
2. Image wrapping to add hover expansion svg done on server

Pros:

• Hover on images showing expand button is available immediately once html is downloaded to client.

Cons

• architecture complexity. Part client/part server
• possibility of disk-read failing, need to handle errors

What do you think about having everything be done on the client-side?

[xvandish]

@afischer - pure client side impl made, still very fast, whole thing takes abt ~1ms after dom load. let me know what you think!

[afischer]

I think that 1ms is likely fine! It would probably take just as long for the event handlers to bind client-side anyway, plus I don't think people are quite that fast when interacting with paged 😄

[afischer]

[We spoke about this offline but posting here for posterity]

This looks awesome, just one small thing: it seems like since the code for the modal is inlined in the footer, it's causing a TypeError on the homepage. If we could either add a more tightly scoped check or refactor this a bit to only appear on "category"/article pages (bad name, I know!) this should be good to merge!

This is also another good case for consideration when working on #53. Might add a note there about thinking about inlined client-side code.

[xvandish]

good catch!
q for you @afischer - Are there any pages besides the homepage that aren't "category" article pages?

I can think of two ways rn

1. Move this script to the /layouts/categories/default.ejs file
2. leave code in footer, refactor a bit + add a check like if(window.location.pathname !== '/') { initializeModal() }

Although, if we go with 1 - we've now split <script> tags to 3 different places, which could get gnarly.

I also gave whats currently in partials/head.ejsa quick look - I think all the scripts could easily move down to the footer, which would favor option 2 (as long the locals.inlineCSS var is also available to the footer tamplate).

My suggestion would be option 2 - leave it in footer and check hostname before initializing - and I can open a separate PR to shift the other scripts down to footer as well to address #53.

What do you think?

[afischer]

2 seems like an easy and effective fix!

A third option, which might be the cleanest, could be to pull the HTML content from categories/default.ejs and the the footer JS into a modal.ejs partial, and include that at the bottom of the body of the categories/default.ejs teamplate. This would keep all modal code together while allowing the script to run after the important article content paint since it would still be down at the bottom of the body.

[xvandish]

Thats an even better idea! I've made that change, and there's now a layouts/partials/imgModal.ejs file, which is imported in layouts/categories/default.ejs.

[afischer]

Awesome, thanks so much for implementing this and for the quick work! Looking forward to getting this into our next release!