ADAPT 1445: Gallery Slideshow

[nplowman]

READY FOR REVIEW

Summary

• Provides a new Gallery Slideshow component.

Review By (Date)

• Within next 1-2 days.

Criticality

• Medium (Probably 6/10)

Review Tasks

Setup tasks and/or behavior to test

If testing locally....

1. Check out this branch
2. Run netlify dev
3. Navigate to http://localhost:8000/test-items/nathan/gallery-examples

If testing on Preview environment...
Go to: https://deploy-preview-192--adapt-giving.netlify.app/editor/?access_key=AKM65vSq8a6vf2HVvWH8zwtt&path=test-items/nathan/gallery-examples&_storyblok=46950529&_storyblok_c=page&_storyblok_tk%5Bspace_id%5D=78141&_storyblok_tk%5Btimestamp%5D=1619717366&_storyblok_tk%5Btoken%5D=039101236669104a2e5d5317b830d32bd36ba209&_storyblok_version=&_storyblok_lang=default&_storyblok_release=0

Front End Validation

• Is the markup using the appropriate semantic tags and passes HTML validation?
• Cross-browser testing has been performed?
• Automated accessibility scans performed?
• Manual accessibility tests performed?
• Design is approved by @ user?

Backend / Functional Validation

Code

• Are the naming conventions following our standards?
• Does the code have sufficient inline comments?
• Is there anything in this code that would be hidden or hard to discover through the UI?
• Are there any code smells?
• Are tests provided? eg (unit, behat, or codeception)

Code security

• Are all forms properly sanitized?
• Any obvious security flaws or new areas for attack?

General

• Is there anything included in this PR that is not related to the problem it is trying to solve?
• Is the approach to the problem appropriate?

Affected Projects or Products

• Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

• https://stanfordits.atlassian.net/browse/ADAPT-1445

Resources

• Docs for Plugin: https://react-slick.neostack.com/docs/get-started
• Figma specs: https://www.figma.com/file/B36rIlaDHzQKwrmJccZpCO/OOD-Giving-Design-System-(SWS)?node-id=17%3A922

Notes

• This component uses a modal. The Search Overlay is very similar but was not built in a reusable way, so I created a new modal component based on this that could be reused in other contexts.

[sherakama]

@yvonnetangsu Please have a review as well.

[sherakama]

Nice work. Good choice on using the slick slider and thank you for using our existing resources. A few questions were left about the focus states and keyboard controls but overall this is functioning very well. I will let @yvonnetangsu bring her review for more feedback. One minor detail I noticed was that the gradient overlay on the next thumbnail gets lost on some mobile sizes after using the prev/next controls. Thanks.

[yvonnetangsu]

@nplowman @sherakama This looks great. Nice job overall! I'll do a more in-depth review tomorrow, but here are a few suggestions for now:

• Please add a scroll lock to the page elements that's behind the modal overlay (please see the search overlay component, there might be something reusable there - and as Shea pointed out, the focus trap hook used on the search overlay could probably be used as well)
• For the control buttons - expand, next, prev etc, please add the regular hand pointer cursor.
• I noticed that when I go from one slide to the next, if the number of lines changes in the caption, the elements beneath shift with it as well. I'll check with the designers to see if they want to clamp the # of lines in the caption and only show the full caption in the expanded view, or there might be other better solutions.
• Please add an underline to the word "Close" on hover/focus for the modal Close button.

[nplowman]

All fixes should be implemented now, except for:

1. The focus element after modal is opened. (Waiting to hear back from SODA).
2. "One minor detail I noticed was that the gradient overlay on the next thumbnail gets lost on some mobile sizes after using the prev/next controls." - Need to look into this a bit more. The Figma specs described simply having overlay on the last item, but I ended up making it appear conditionally, as it looked broken having the overlay when the last item was selected. The issue Shea is describing is probably related to this.

A few other notes:

1. For the scroll lock: I initially tried to just reuse the SearchOverlayContext which provides this behavior, but found it was too tightly coupled to the search implementation. So I just borrowed it's approach and moved it into the new reusable Modal component. I made one adjustment and removed the "position: fixed" style rule, as it scrolls the user to the top of the page, which isn't desirable. And the scroll lock seems to work fine without it.

2. For the issue with the variable caption height: I'd need to make some adjustments, but one option could be setting the height of the caption container to fit the longest caption. This would prevent things from "jumping" as it resizes. However, it would leave empty white space for the shorter captions.

[yvonnetangsu]

@nplowman Thank you for the changes! I still owe you a more in-depth review (was in meetings all day, but I'll get to it today (thursday)).

• Thanks for adding the focus states!
• If you could please add the hand cursor to the thumbnails also, that'd be great.
• Heads up SODA might want the thumbnails turned into html button elements. There's a chance that they would like these to be keyboard focusable - but let's wait for a11y review at the end. So no action needed for this for now, just a FYI.
• When I went to the preview link and open the modal, I don't seem to see a focus trap - I was able to tab out of the modal to the page elements behind.
• If I hit "escape" to close the modal, the focus land on a different "expand" button (from different set of gallery slides 😄 ), but if I hit "enter" when the focus is on the close button, then it behaves as intended (focus lands on initial expand button).

More later today. Thanks in advance!

[nplowman]

@yvonnetangsu

1. Hand cursor has been added to thumbnails.
2. Focus trap has been added. The existing focus trap hook requires passing in first and last focusable items, so I also added the "tabbable" library which helps us find this given a parent element. This is useful for the cases like this one, where the last tabbable element may be in a child element and it would be difficult to attach a ref.
3. The bug with the escape button has been resolved. For some reason, calling the callback prop directly was causing mix-ups between component instances, so switched to triggering button click programmatically.

[yvonnetangsu]

@yvonnetangsu

1. Hand cursor has been added to thumbnails.
2. Focus trap has been added. The existing focus trap hook requires passing in first and last focusable items, so I also added the "tabbable" library which helps us find this given a parent element. This is useful for the cases like this one, where the last tabbable element may be in a child element and it would be difficult to attach a ref.
3. The bug with the escape button has been resolved. For some reason, calling the callback prop directly was causing mix-ups between component instances, so switched to triggering button click programmatically.

Thanks, @nplowman. All working great.
I realize I haven't replied to you about the caption length/jumping issue. I like your suggestion (finding the longest caption and set the height of the gallery using that. Let me check with the design team and see if they prefer this solution or something else.

[yvonnetangsu]

@nplowman I checked with the team on the height jump issue. The decision is to hold off on making changes until we receive client feedback. Thanks!

[sherakama]

This is looking great. One more detail with the focus trap is that it should allow the end-user to tab into the comments to focus on links there. Right now it is just cycling through the three controls.

[nplowman]

Thanks, @nplowman. All working great.
I realize I haven't replied to you about the caption length/jumping issue. I like your suggestion (finding the longest caption and set the height of the gallery using that. Let me check with the design team and see if they prefer this solution or something else.

@yvonnetangsu
I was hoping this would be as simple as adding height: min-content but forgot that CSS doesn't include hidden items in the calculations. So this could be a bit more complicated than I thought.

[sherakama]

Updates are in review with Bhavika.

[yvonnetangsu]

@nplowman Tiny request - Could you please add some spacing above the "Close" button when it's in modal mode? (Probably matching the search modal spacing would be great)

[nplowman]

@yvonnetangsu Just pushed out a fix for the padding. Take a look and see what you think...
On my screen size, the added padding makes it so I have to scroll vertically to see the full carousel now, which isn't ideal. But we could possibly adjust for that by reducing gallery size.

[yvonnetangsu]

This is looking great. One more detail with the focus trap is that it should allow the end-user to tab into the comments to focus on links there. Right now it is just cycling through the three controls.

@nplowman Not sure if @sherakama's comment got buried - can confirm that I'm seeing this also.

@yvonnetangsu - I just pushed out a fix for this. The last element for the focus trap was getting set when the modal initialized, so it was having issues when the slider content changed. It has been tweaked to update whenever the child content changes.

@yvonnetangsu Just pushed out a fix for the padding. Take a look and see what you think...
On my screen size, the added padding makes it so I have to scroll vertically to see the full carousel now, which isn't ideal. But we could possibly adjust for that by reducing gallery size.

@nplowman Good point, the gallery is a lot taller than the search bar. Please feel free to reduce the padding as you see fit. As long as there's a bit of space above the close button, it's totally fine. Thanks!

[yvonnetangsu]

@Lyafel If you could please add similar sr-only text to the Quote Slider counter like Nathan did here, that'd be awesome.

[nplowman]

And, if we could add an option to accommodate when this Gallery is placed in a section with the fog-light grey background color, that'd be awesome. It'd be nice for that thumbnail gradient to blend in for the fog-light background (currently works beautifully for white background).

I've added a "BackgroundColor" field to the component using the same data source that was being used on other components. If "fog light" is selected, the background of the overlay will also be adjusted appropriately.

[yvonnetangsu]

And, if we could add an option to accommodate when this Gallery is placed in a section with the fog-light grey background color, that'd be awesome. It'd be nice for that thumbnail gradient to blend in for the fog-light background (currently works beautifully for white background).

[yvonnetangsu]

Added a few comments - thanks, @nplowman !

[yvonnetangsu]

@nplowman Thank you for all the changes - perfect. I added a comment about Bhavika's gradient overlay request in the JIRA ticket. Also, are we going to go with having only 1 caption position option now (SODA recommendation)? If that decision is confirmed, then please remove that field in Storyblok. Thanks!

[yvonnetangsu]

One more - apologize about the comments trickling in like this. Last one for today, I promise 😂

For the Gallery width "Constrain Max Width" option, we need to move the centered-container class to the parent container of the element with the flex-lg-8-of-12 type classes, since centered-container overrides the width and we'll always end up getting 12 of 12 columns. What we'd like is that, centered-container sets the bounding width for 12 of 12 columns for all breakpoints, then the width classes determines how many columns the gallery actually takes up.

Here's an example (here I'm moving the centered-container class to the parent "section" element and it seems to do the trick:

Thanks, @nplowman !

[yvonnetangsu]

@nplowman Thank you for the changes - I've tested all the use cases and they all look great to me. @bhavikasolanki might want a final look at the expand view changes. Please feel free to tag her for visual QA or I can ping her also. Thanks again!

[yvonnetangsu]

I'm pre-approving, but Bhavika would probably want another look. @sherakama do you have anything to add?

[sherakama]

GTG by me.