Use thumbnail images in Screenshots component #5337

willdurand · 2018-06-20T13:26:33Z

Fix mozilla/addons#11923

This PR fixes the Screenshots component to use the thumbnails supplied by the API.

I have also added Flow coverage and more tests. When adding Flow coverage, Flow told me that ClientRect did not have a x attribute, which is correct according to https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect. That's why I changed x by left in this patch.

There is one more thing I'd like to do: moving the code (CSS + JS) into a directory named Screenshots. I did not do it yet because the diff would be unreadable, but that would be cool to do it before merging this PR, I guess.

codecov-io · 2018-06-20T13:38:27Z

Codecov Report

Merging #5337 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#5337      +/-   ##
==========================================
+ Coverage   96.97%   97.06%   +0.09%     
==========================================
  Files         215      215              
  Lines        5382     5553     +171     
  Branches     1049     1089      +40     
==========================================
+ Hits         5219     5390     +171     
  Misses        145      145              
  Partials       18       18

Impacted Files	Coverage Δ
src/amo/components/ScreenShots/index.js	`100% <100%> (ø)`
src/amo/sagas/addonsByAuthors.js	`100% <0%> (ø)`	⬆️
src/core/utils/index.js	`100% <0%> (ø)`	⬆️
src/amo/reducers/addonsByAuthors.js	`100% <0%> (ø)`	⬆️
src/core/constants.js	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec898e6...1bc5f0f. Read the comment docs.

kumar303

EEsh, my eyes. I didn't know we had some bad hacks in here. Oh well.

Thanks for the clean-up and Flow/test coverage. I mainly requested minor changes.

kumar303 · 2018-06-20T18:54:37Z

tests/unit/amo/components/TestScreenShots.js

+        pageYOffset: 20,
+      };
+
+      const bounds = getThumbBoundsFn(0, fakeDocument, fakeWindow);


I know this signature is convenient but it's hard to maintain calling code like this. I would rather see it done with a a parameter object:

const bounds = getThumbBoundsFn(0, { _document: fakeDocument, _window: fakeWindow, });

Otherwise it's too easy to make mistakes around positional arguments when refactoring without Flow. If we had Flow coverage on the test file I'd be less worried about it but we do not.

sure, that works for me!

kumar303 · 2018-06-20T19:04:14Z

src/amo/components/ScreenShots.js

-    const thumbnail = document.querySelectorAll('.pswp-thumbnails')[index];
+  getThumbBoundsFn: (
+    index: number,
+    _document: typeof document = document,


This should be _document = typeof document !== 'undefined' ? document : null to make sure it won't crash with a ReferenceError in SSR mode. I'm not sure that SSR code would trigger it but we should make it safe just in case.

mmh, that was to typehint the parameter, not to check for undefined.
I should still protect against undefined references though.

Note: I did use typeof document instead of Document, which is not white-listed for ESLint and because I was using typeof window for the _window parameter.

kumar303 · 2018-06-20T19:04:41Z

src/amo/components/ScreenShots.js

+  getThumbBoundsFn: (
+    index: number,
+    _document: typeof document = document,
+    _window: typeof window = window


This should also be _window = typeof window !== 'undefined' ? window : null to avoid a ReferenceError in SSR mode.

kumar303 · 2018-06-20T19:05:50Z

src/amo/components/ScreenShots.js

  // https://github.com/minhtranite/react-photoswipe/issues/23
-  getThumbBoundsFn: /* istanbul ignore next */ function getThumbBoundsFn(index) {


Thanks for adding test coverage ❤️

kumar303 · 2018-06-20T19:08:37Z

src/amo/components/ScreenShots.js

@@ -17,59 +23,100 @@ const PHOTO_SWIPE_OPTIONS = {
  counterEl: true,
  arrowEl: true,
  preloaderEl: true,
-  // Overload getThumbsBoundsFn as workaround to
+  // Overload getThumbBoundsFn as workaround to
  // https://github.com/minhtranite/react-photoswipe/issues/23


Ouch. This hack is really unfortunate and will be fragile for us to maintain. It looks like a fix may land soon: minhtranite/react-photoswipe#36

A year ago and still nothing.. Mid-term plan is to replace this gallery anyway, because it does not look super nice and we could do better.

kumar303 · 2018-06-20T19:18:18Z

src/amo/components/ScreenShots.js

-const formatPreviews = (previews) => (
+type ExternalPreview = {|
+  caption: string,
+  image_size: [number, number],


It would be nice to name these like image_size: [width: number, height: number] (or w and h would be fine)

ah snap, there is no named tuples in Flow, so we cannot name the elements in the array.

kumar303 · 2018-06-20T19:18:41Z

src/amo/components/ScreenShots.js

+  caption: string,
+  image_size: [number, number],
+  image_url: string,
+  thumbnail_size: [number, number],


Same here: thumbnail_size: [width: number, height: number]

kumar303 · 2018-06-20T19:20:58Z

src/amo/components/ScreenShots.js

  }

+  viewport: HTMLElement | null;


I think it's nice to put class variable types at the top of the class. We don't have a rule about it though.

Me too but ESLint complains.. Maybe time to disable the react/sort-comp rule globally?

Gah! react/sort-comp is the worst. I always disable it. Yeah, we should just disable it globally.

Opened #5357

kumar303 · 2018-06-20T19:22:49Z

tests/unit/amo/components/TestScreenShots.js

+
+      const bounds = getThumbBoundsFn(0, fakeDocument);
+
+      expect(bounds).toBeInstanceOf(Object);


I don't think you need this because .toEqual() below it will produce an error saying that it did not receive an Object.

kumar303 · 2018-06-20T19:31:48Z

Moving this to a ScreenShots directory as the last step before merging sounds like a good idea.

willdurand · 2018-06-20T21:52:25Z

src/amo/components/ScreenShots.js

-  getThumbBoundsFn: /* istanbul ignore next */ function getThumbBoundsFn(index) {
-    const thumbnail = document.querySelectorAll('.pswp-thumbnails')[index];
+  getThumbBoundsFn: (index: number, {
+    _document = typeof document !== 'undefined' ? document : null,


@kumar303 Flow complains about this line because the maybe/union type typeof document | null does not work. I don't know why and I am stuck right now.

Looks like a Flow bug that hasn't been fixed: facebook/flow#183

kumar303

⚡️

willdurand force-pushed the screenshot-thumbnail branch from ae6c737 to 528c831 Compare June 20, 2018 13:38

willdurand force-pushed the screenshot-thumbnail branch from 528c831 to 140314d Compare June 20, 2018 13:41

willdurand changed the title ~~[WIP] Use thumbnail images in Screenshots component~~ Use thumbnail images in Screenshots component Jun 20, 2018

willdurand requested a review from kumar303 June 20, 2018 14:21

willdurand force-pushed the screenshot-thumbnail branch from 6daa703 to aafb848 Compare June 20, 2018 14:46

kumar303 suggested changes Jun 20, 2018

View reviewed changes

willdurand commented Jun 20, 2018

View reviewed changes

willdurand force-pushed the screenshot-thumbnail branch from 6a70457 to ef462ae Compare June 21, 2018 10:14

willdurand requested a review from kumar303 June 21, 2018 10:16

willdurand added 3 commits June 21, 2018 12:48

Use thumbnail images in Screenshots component

af13f57

Add more tests

d1a9c10

fixes

b0ff064

willdurand force-pushed the screenshot-thumbnail branch from ef462ae to b0ff064 Compare June 21, 2018 10:48

kumar303 approved these changes Jun 21, 2018

View reviewed changes

move file

1bc5f0f

willdurand merged commit b083e8f into master Jun 21, 2018

willdurand deleted the screenshot-thumbnail branch June 21, 2018 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use thumbnail images in Screenshots component #5337

Use thumbnail images in Screenshots component #5337

willdurand commented Jun 20, 2018 •

edited

Loading

codecov-io commented Jun 20, 2018 •

edited

Loading

kumar303 left a comment

kumar303 Jun 20, 2018

willdurand Jun 20, 2018

kumar303 Jun 20, 2018

willdurand Jun 20, 2018 •

edited

Loading

kumar303 Jun 20, 2018

kumar303 Jun 20, 2018

kumar303 Jun 20, 2018

willdurand Jun 20, 2018

kumar303 Jun 20, 2018

willdurand Jun 20, 2018

kumar303 Jun 20, 2018

kumar303 Jun 20, 2018

willdurand Jun 20, 2018

kumar303 Jun 21, 2018

willdurand Jun 21, 2018

kumar303 Jun 20, 2018

kumar303 commented Jun 20, 2018

willdurand Jun 20, 2018

willdurand Jun 21, 2018

kumar303 left a comment

		// https://github.com/minhtranite/react-photoswipe/issues/23
		getThumbBoundsFn: /* istanbul ignore next */ function getThumbBoundsFn(index) {


		const bounds = getThumbBoundsFn(0, fakeDocument);

		expect(bounds).toBeInstanceOf(Object);

Use thumbnail images in Screenshots component #5337

Use thumbnail images in Screenshots component #5337

Conversation

willdurand commented Jun 20, 2018 • edited Loading

codecov-io commented Jun 20, 2018 • edited Loading

Codecov Report

kumar303 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

willdurand Jun 20, 2018 • 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

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

kumar303 commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 left a comment

Choose a reason for hiding this comment

willdurand commented Jun 20, 2018 •

edited

Loading

codecov-io commented Jun 20, 2018 •

edited

Loading

willdurand Jun 20, 2018 •

edited

Loading