From 412e428c3e82264dab4fbcfde2ccc179a7069ec5 Mon Sep 17 00:00:00 2001 From: William Durand Date: Tue, 2 Mar 2021 18:35:46 +0100 Subject: [PATCH 1/2] Replace photoswipe react lib --- jest.config.js | 2 - package.json | 3 +- src/amo/components/ScreenShots/index.js | 119 ++++++------- src/amo/components/ScreenShots/styles.scss | 19 +-- tests/unit/amo/components/TestScreenShots.js | 166 +++---------------- yarn.lock | 24 +-- 6 files changed, 87 insertions(+), 246 deletions(-) diff --git a/jest.config.js b/jest.config.js index c8d55e48fa8..733a9757535 100644 --- a/jest.config.js +++ b/jest.config.js @@ -8,8 +8,6 @@ module.exports = { moduleDirectories: ['src', 'node_modules'], moduleFileExtensions: ['js', 'json', 'jsx'], moduleNameMapper: { - // Prevent un-transpiled react-photoswipe code being required. - '^photoswipe$': '/node_modules/photoswipe', // Alias tests for tests to be able to import helpers. '^tests/(.*)$': '/tests/$1', // Replaces the following formats with an empty module. diff --git a/package.json b/package.json index db459d9a327..a5260cbf4eb 100644 --- a/package.json +++ b/package.json @@ -186,6 +186,7 @@ "nano-time": "1.0.0", "normalize.css": "8.0.1", "photon-colors": "3.3.2", + "photoswipe": "4.1.3", "pino": "6.11.1", "pino-mozlog": "2.2.0", "prop-types": "15.7.2", @@ -203,7 +204,7 @@ "react-keydown": "1.9.12", "react-nested-status": "0.2.1", "react-onclickoutside": "6.10.0", - "react-photoswipe": "1.3.0", + "react-photoswipe-gallery": "1.3.4", "react-redux": "5.1.2", "react-router": "4.3.1", "react-router-dom": "4.3.1", diff --git a/src/amo/components/ScreenShots/index.js b/src/amo/components/ScreenShots/index.js index 821cf72a5a6..647ba9d0940 100644 --- a/src/amo/components/ScreenShots/index.js +++ b/src/amo/components/ScreenShots/index.js @@ -1,26 +1,13 @@ /* @flow */ -/* global window */ -import invariant from 'invariant'; import * as React from 'react'; -import { PhotoSwipeGallery } from 'react-photoswipe'; -import 'react-photoswipe/lib/photoswipe.css'; +import { Gallery, Item } from 'react-photoswipe-gallery'; +import invariant from 'invariant'; +import 'photoswipe/dist/photoswipe.css'; +import 'photoswipe/dist/default-skin/default-skin.css'; import type { PreviewType } from 'amo/types/addons'; import './styles.scss'; -type ThumbBounds = - | false - | {| - w: number, - x: number, - y: number, - |}; - -type GetThumbBoundsExtraParams = {| - _document: typeof document | null, - _window: typeof window | null, -|}; - export const PHOTO_SWIPE_OPTIONS = { closeEl: true, captionEl: true, @@ -30,67 +17,32 @@ export const PHOTO_SWIPE_OPTIONS = { counterEl: true, arrowEl: true, preloaderEl: true, - // Overload getThumbBoundsFn as workaround to - // https://github.com/minhtranite/react-photoswipe/issues/23 - getThumbBoundsFn: ( - index: number, - { - // $FlowFixMe: see https://github.com/facebook/flow/issues/183 - _document = typeof document !== 'undefined' ? document : null, - _window = typeof window !== 'undefined' ? window : null, - }: GetThumbBoundsExtraParams = {}, - ): ThumbBounds => { - if (!_document || !_window) { - return false; - } - - const thumbnail = _document.querySelectorAll('.pswp-thumbnails')[index]; - - if (thumbnail && thumbnail.getElementsByTagName) { - const img = thumbnail.getElementsByTagName('img')[0]; - const pageYScroll = - _window.pageYOffset || - (_document.documentElement ? _document.documentElement.scrollTop : 0); - const rect = img.getBoundingClientRect(); - - return { x: rect.left, y: rect.top + pageYScroll, w: rect.width }; - } - - return false; - }, }; -export const thumbnailContent = (item: PreviewType): React.Node => ( - {item.title} -); - type Props = {| previews: Array, |}; export default class ScreenShots extends React.Component { - onClose = (photoswipe: Object) => { - const index = photoswipe.getCurrentIndex(); + viewport: HTMLElement | null; + onOpenPhotoswipe = (photoswipe: Object) => { invariant(this.viewport, 'viewport ref is required'); - const list = this.viewport.querySelector('.pswp-thumbnails'); - + const list = this.viewport.querySelector('.ScreenShots-list'); invariant(list, 'list is required'); - const currentItem = list.children[index]; - const offset = currentItem.getBoundingClientRect().left; - list.scrollLeft += offset - list.getBoundingClientRect().left; - }; + // This is used to update the horizontal list of thumbnails in order to + // show the last image displayed in fullscreen mode when we close the + // carousel. + photoswipe.listen('close', () => { + const index = photoswipe.getCurrentIndex(); + const currentItem = list.children[index]; + const offset = currentItem.getBoundingClientRect().left; - viewport: HTMLElement | null; + list.scrollLeft += offset - list.getBoundingClientRect().left; + }); + }; render() { const { previews } = this.props; @@ -103,13 +55,36 @@ export default class ScreenShots extends React.Component { this.viewport = el; }} > - +
+ + {previews.map((preview) => ( + + {({ ref, open }) => ( + // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions + + )} + + ))} + +
); diff --git a/src/amo/components/ScreenShots/styles.scss b/src/amo/components/ScreenShots/styles.scss index e0484e75551..7197417ba66 100644 --- a/src/amo/components/ScreenShots/styles.scss +++ b/src/amo/components/ScreenShots/styles.scss @@ -7,10 +7,6 @@ $image-height: $screenshot-height - 20px; $screenshot-width: 320px; .ScreenShots { - .pswp-thumbnails { - line-height: 0; - } - .pswp__caption__center { text-align: center; } @@ -20,9 +16,10 @@ $screenshot-width: 320px; height: $screenshot-height; } -.ScreenShots-list .pswp-thumbnails { +.ScreenShots-list { display: flex; height: $screenshot-height; + line-height: 0; list-style-type: none; margin: 0; overflow-x: scroll; @@ -31,22 +28,18 @@ $screenshot-width: 320px; width: auto; } -.ScreenShots-list .pswp-thumbnail { +.ScreenShots-image { + cursor: pointer; display: inline-block; + height: $image-height; margin: 0 10px; + width: $screenshot-width; &:first-of-type { @include margin-start(0); } } -.ScreenShots-image { - cursor: pointer; - display: inline-block; - height: $image-height; - width: $screenshot-width; -} - // Don't stretch the images. .ScreenShots-image, .pswp__img { diff --git a/tests/unit/amo/components/TestScreenShots.js b/tests/unit/amo/components/TestScreenShots.js index b6a9c4c6539..330308141d9 100644 --- a/tests/unit/amo/components/TestScreenShots.js +++ b/tests/unit/amo/components/TestScreenShots.js @@ -1,11 +1,8 @@ import { shallow, mount } from 'enzyme'; import * as React from 'react'; -import { PhotoSwipeGallery } from 'react-photoswipe'; +import { Gallery, Item } from 'react-photoswipe-gallery'; -import ScreenShots, { - PHOTO_SWIPE_OPTIONS, - thumbnailContent, -} from 'amo/components/ScreenShots'; +import ScreenShots, { PHOTO_SWIPE_OPTIONS } from 'amo/components/ScreenShots'; const HEIGHT = 400; const WIDTH = 200; @@ -34,36 +31,25 @@ describe(__filename, () => { it('renders the previews', () => { const root = shallow(); - const gallery = root.children().children(); - expect(gallery.type()).toEqual(PhotoSwipeGallery); - expect(gallery.prop('items')).toEqual(previews); - expect(gallery.prop('thumbnailContent')).toEqual(thumbnailContent); - }); - - it('renders custom thumbnail', () => { - const thumbnailSrc = 'http://example.com/thumbnail.png'; - const thumbnailHeight = 123; - const thumbnailWidth = 200; - - const item = { - src: 'https://foo.com/img.png', - title: 'test title', - h: HEIGHT, - w: WIDTH, - thumbnail_src: thumbnailSrc, - thumbnail_h: thumbnailHeight, - thumbnail_w: thumbnailWidth, - }; - - const thumbnail = shallow(thumbnailContent(item)); - - expect(thumbnail.type()).toEqual('img'); - expect(thumbnail.prop('src')).toEqual(thumbnailSrc); - expect(thumbnail.prop('height')).toEqual(thumbnailHeight); - expect(thumbnail.prop('width')).toEqual(thumbnailWidth); - expect(thumbnail.prop('alt')).toEqual('test title'); - expect(thumbnail.prop('title')).toEqual('test title'); + expect(root.find(Gallery)).toHaveLength(1); + expect(root.find(Gallery)).toHaveProp('options', PHOTO_SWIPE_OPTIONS); + expect(root.find(Item)).toHaveLength(previews.length); + + previews.forEach((preview, index) => { + const item = root.find(Item).at(index); + expect(item).toHaveProp('original', preview.src); + expect(item).toHaveProp('thumbnail', preview.thumbnail_src); + expect(item).toHaveProp('width', preview.w); + expect(item).toHaveProp('height', preview.h); + expect(item).toHaveProp('title', preview.title); + + const image = item.shallow(); + expect(image.type()).toEqual('img'); + expect(image).toHaveProp('src', preview.thumbnail_src); + expect(image).toHaveProp('width', preview.thumbnail_w); + expect(image).toHaveProp('height', preview.thumbnail_h); + }); }); it('scrolls to the active item on close', () => { @@ -83,114 +69,12 @@ describe(__filename, () => { }; sinon.stub(root.instance().viewport, 'querySelector').returns(list); - const photoswipe = { getCurrentIndex: () => 1 }; - root.instance().onClose(photoswipe); + const fakePhotoswipe = { + getCurrentIndex: () => 1, + listen: (event, callback) => callback(), + }; + root.instance().onOpenPhotoswipe(fakePhotoswipe); // 0 += 500 - 55 expect(list.scrollLeft).toEqual(445); }); - - describe('PHOTO_SWIPE_OPTIONS.getThumbBoundsFn', () => { - const { getThumbBoundsFn } = PHOTO_SWIPE_OPTIONS; - - const getFakeDocument = ({ left, top, width }) => { - const fakeImg = { - getBoundingClientRect: () => ({ - height: 123, - left, - top, - width, - }), - }; - - const fakeThumbnail = { - getElementsByTagName: () => [fakeImg], - }; - - const fakeDocument = { - querySelectorAll: () => [fakeThumbnail], - }; - - return fakeDocument; - }; - - it('returns false if thumbnail does not exist', () => { - const bounds = getThumbBoundsFn(0); - - expect(bounds).toEqual(false); - }); - - it('returns false if _document is null', () => { - const bounds = getThumbBoundsFn(0, { _document: null }); - - expect(bounds).toEqual(false); - }); - - it('returns false if _window is null', () => { - const bounds = getThumbBoundsFn(0, { - _document: getFakeDocument({ left: 1, top: 2, width: 3 }), - _window: null, - }); - - expect(bounds).toEqual(false); - }); - - it('returns an object with x, y and w values', () => { - const left = 123; - const top = 124; - const width = 100; - - const fakeDocument = getFakeDocument({ left, top, width }); - - const bounds = getThumbBoundsFn(0, { _document: fakeDocument }); - - expect(bounds).toEqual({ - w: width, - x: left, - y: top, - }); - }); - - it('uses window.pageYOffset to compute `y` if available', () => { - const left = 123; - const top = 124; - const width = 100; - - const fakeDocument = getFakeDocument({ left, top, width }); - - const fakeWindow = { - pageYOffset: 20, - }; - - const bounds = getThumbBoundsFn(0, { - _document: fakeDocument, - _window: fakeWindow, - }); - - expect(bounds).toEqual({ - w: width, - x: left, - y: top + fakeWindow.pageYOffset, - }); - }); - - it('uses document.documentElement.scrollTop to compute `y` if available', () => { - const left = 123; - const top = 124; - const width = 100; - const scrollTop = 30; - - const fakeDocument = getFakeDocument({ left, top, width }); - fakeDocument.documentElement = { - scrollTop, - }; - - const bounds = getThumbBoundsFn(0, { _document: fakeDocument }); - - expect(bounds).toEqual({ - w: width, - x: left, - y: top + scrollTop, - }); - }); - }); }); diff --git a/yarn.lock b/yarn.lock index 7fcce3067bb..be872a356b7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3355,7 +3355,7 @@ class-utils@^0.3.5: isobject "^3.0.0" static-extend "^0.1.1" -classnames@2.2.6, classnames@2.x, classnames@^2.2.1, classnames@^2.2.3, classnames@^2.2.6: +classnames@2.2.6, classnames@2.x, classnames@^2.2.1, classnames@^2.2.6: version "2.2.6" resolved "https://registry.yarnpkg.com/classnames/-/classnames-2.2.6.tgz#43935bffdd291f326dad0a205309b38d00f650ce" integrity sha512-JR/iSQOSt+LQIWwrwEzJ9uk0xfN3mTVYMwt1Ir5mUcSN6pU+V4zQFFaJsclJbPuAUQH+yfWef6tm7l1quW3C8Q== @@ -8192,11 +8192,6 @@ lodash.memoize@^4.1.2: resolved "https://registry.yarnpkg.com/lodash.memoize/-/lodash.memoize-4.1.2.tgz#bcc6c49a42a2840ed997f323eada5ecd182e0bfe" integrity sha1-vMbEmkKihA7Zl/Mj6tpezRguC/4= -lodash.pick@^4.2.1: - version "4.4.0" - resolved "https://registry.yarnpkg.com/lodash.pick/-/lodash.pick-4.4.0.tgz#52f05610fff9ded422611441ed1fc123a03001b3" - integrity sha1-UvBWEP/53tQiYRRB7R/BI6AwAbM= - lodash.sortby@^4.7.0: version "4.7.0" resolved "https://registry.yarnpkg.com/lodash.sortby/-/lodash.sortby-4.7.0.tgz#edd14c824e2cc9c1e0b0a1b42bb5210516a42438" @@ -9771,7 +9766,7 @@ photon-colors@3.3.2: resolved "https://registry.yarnpkg.com/photon-colors/-/photon-colors-3.3.2.tgz#eaf2e5a8ba9368fcdee0607cc86a9f613e6d3417" integrity sha512-xCeL7J2F8cjM00zQZEZawHAGnrSOM509RbanL4c8hvrV8n19V/wwdzydX6rSUEtLYj4nx4OvhmKC4/vujo9f/Q== -photoswipe@^4.1.0: +photoswipe@4.1.3: version "4.1.3" resolved "https://registry.yarnpkg.com/photoswipe/-/photoswipe-4.1.3.tgz#59f49494eeb9ddab5888d03392926a19bc197550" integrity sha512-89Z43IRUyw7ycTolo+AaiDn3W1EEIfox54hERmm9bI12IB9cvRfHSHez3XhAyU8XW2EAFrC+2sKMhh7SJwn0bA== @@ -10521,7 +10516,7 @@ prop-types-exact@^1.2.0: object.assign "^4.1.0" reflect.ownkeys "^0.2.0" -prop-types@15.7.2, prop-types@^15.5.10, prop-types@^15.6.1, prop-types@^15.6.2, prop-types@^15.7.2: +prop-types@15.7.2, prop-types@^15.6.1, prop-types@^15.6.2, prop-types@^15.7.2: version "15.7.2" resolved "https://registry.yarnpkg.com/prop-types/-/prop-types-15.7.2.tgz#52c41e75b8c87e72b9d9360e0206b99dcbffa6c5" integrity sha512-8QQikdH7//R2vurIJSutZ1smHYTcLpRWEOlHnzcWHmBYrOGUysKwSsrC89BCiFj3CbrfJ/nXFdJepOVrY1GCHQ== @@ -10888,15 +10883,10 @@ react-onclickoutside@6.10.0: resolved "https://registry.yarnpkg.com/react-onclickoutside/-/react-onclickoutside-6.10.0.tgz#05abb592575b08b4d129003494056b9dff46eeeb" integrity sha512-7i2L3ef+0ILXpL6P+Hg304eCQswh4jl3ynwR71BSlMU49PE2uk31k8B2GkP6yE9s2D4jTGKnzuSpzWxu4YxfQQ== -react-photoswipe@1.3.0: - version "1.3.0" - resolved "https://registry.yarnpkg.com/react-photoswipe/-/react-photoswipe-1.3.0.tgz#016dd978450a8406776db97511eaf96f2ffb9cfb" - integrity sha512-1ok6vXFAj/rd60KIzF0YwCdq1Tcl+8yKqWJHbPo43lJBuwUi+LBosmBdJmswpiOzMn2496ekU0k/r6aHWQk7PQ== - dependencies: - classnames "^2.2.3" - lodash.pick "^4.2.1" - photoswipe "^4.1.0" - prop-types "^15.5.10" +react-photoswipe-gallery@1.3.4: + version "1.3.4" + resolved "https://registry.yarnpkg.com/react-photoswipe-gallery/-/react-photoswipe-gallery-1.3.4.tgz#770f82ae7afb3acf4cdc27d08db7a978555b4b3e" + integrity sha512-PiKGQTWHMORlMgEI32e8nMpVBKZeTJv378x4Jom9Dl68HPZdKadEQPcgVkiVPoRqvv0ynHNvFdUehzpQqe8SXw== react-redux@5.1.2: version "5.1.2" From 883e51c034495949933e7080f34ec16565036d50 Mon Sep 17 00:00:00 2001 From: William Durand Date: Wed, 3 Mar 2021 09:40:18 +0100 Subject: [PATCH 2/2] fixes --- src/amo/components/ScreenShots/index.js | 4 ++-- tests/unit/amo/components/TestScreenShots.js | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/amo/components/ScreenShots/index.js b/src/amo/components/ScreenShots/index.js index 647ba9d0940..ae5f38909a4 100644 --- a/src/amo/components/ScreenShots/index.js +++ b/src/amo/components/ScreenShots/index.js @@ -1,7 +1,7 @@ /* @flow */ +import invariant from 'invariant'; import * as React from 'react'; import { Gallery, Item } from 'react-photoswipe-gallery'; -import invariant from 'invariant'; import 'photoswipe/dist/photoswipe.css'; import 'photoswipe/dist/default-skin/default-skin.css'; @@ -72,7 +72,7 @@ export default class ScreenShots extends React.Component { {({ ref, open }) => ( // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions { expect(image).toHaveProp('src', preview.thumbnail_src); expect(image).toHaveProp('width', preview.thumbnail_w); expect(image).toHaveProp('height', preview.thumbnail_h); + expect(image).toHaveProp('alt', preview.title); }); });