Es6 projects common modules gallery lightbox #18113

NataliaLKB · 2017-11-01T14:58:03Z

What does this change?

ES6 conversion for lightbox.js. I tried my best not to rewrite, but just convert (as advised)

This is my first es6 conversion so please let me know everything I could do better :).

Please note, I know of 3 current production bugs in lightbox. They are:

Closing the lightbox doesn't remove the hash from the url (this is now fixed in this conversion)
Share buttons don't work on mobile (not sure why, but I didn't fix as part of this)
Lightbox not working on articles (works now!)

What is the value of this and can you measure success?

More modern JS

Does this affect other platforms - Amp, Apps, etc?

Nope

Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?

Nope

Screenshots

Everything looks the same

Tested in CODE?

Yes

PRBuilds · 2017-11-01T15:06:02Z

PRbuilds results:

Screenshots
desktop.png • tablet.png • mobile.png • wide.png

💚 Exceptions
thrown-exceptions.js

💚 A11y validation
a11y-report.txt

Apache Benchmark Load Testing
loadtesting.txt

💚 Microdata Validation
microdata.txt

--automated message

SiAdcock · 2017-11-01T15:57:44Z

Wow, this one is a monster! Awesome work 🎉

As a general suggestion, how do you feel about adding return type annotations to the methods of the GalleryLightbox class?

SiAdcock · 2017-11-01T16:07:47Z

static/src/javascripts-legacy/projects/common/modules/gallery/lightbox.js

-            this.endslate.endpoint = '/gallery/most-viewed.json';
-            this.endslate.prerender = function () {
-                bonzo(this.elem).addClass(this.componentClass);
-            };


Is the prerender functionality longer needed?

Yes! Not sure how I lost it. I brought it back though :)

SiAdcock · 2017-11-01T16:11:25Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+    });
+};
+
+export { init, GalleryLightbox };


GalleryLightbox doesn't appear to be imported anywhere. Can we remove it from the export object?

It is used in the tests, so I followed the advice in the wiki to export it differently

gu-stav

Oha, this module is painful. Thanks for picking it up! 👍

gu-stav · 2017-11-01T16:10:28Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+        };
+
+        const galleryLightboxHtml: string =
+            `${'<div class="overlay gallery-lightbox gallery-lightbox--closed gallery-lightbox--hover">' +


Since this a template literal you don't have to use concatenation and just use it as a multiline string.

gu-stav · 2017-11-01T16:11:29Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+        // FSM CONFIG
+        this.fsm = new FiniteStateMachine({
+            initial: 'closed',
+            onChangeState(oldState, newState) {


Types? 👍

gu-stav · 2017-11-01T16:12:21Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+
+    generateImgHTML(img: Object, i: number) {
+        const blockShortUrl = config.get('page.shortUrl');
+        const urlPrefix = img.src.indexOf('//') === 0 ? 'http:' : '';


img.src.startsWith('//') ? 'http:' : '';

gu-stav · 2017-11-01T16:14:35Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+        contentEl.style.mozTransform = `translate(${px + offset}px,0)`;
+        contentEl.style.msTransform = `translate(${px + offset}px,0)`;
+        contentEl.style.transform = `translate(${px +
+            offset}px,0) translateZ(0)`;


Does flow complain of you do

Object.assign(contentEl.style, { /* ... */ })

to avoid all the repetition?

It does not! Great suggestions 👍

gu-stav · 2017-11-01T16:15:55Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+        } else if (e.keyCode === 73) {
+            // 'i'
+            this.trigger('toggleInfo');
+        }


🤦‍♀️

gu-stav · 2017-11-01T16:17:18Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+
+            let res;
+
+            bean.on(document.body, 'click', '.js-gallerythumbs', e => {


Because the callback is so big: could you add the :Event type definition here?

gu-stav · 2017-11-01T16:17:53Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+    }
+}
+
+GalleryLightbox.prototype.states = {


Could you move all of these into the constructor()?

gu-stav · 2017-11-01T16:18:20Z

static/src/javascripts/projects/common/modules/gallery/lightbox.js

+            },
+            loadJson(json: GalleryJson) {
+                this.galleryJson = json;
+                this.images = json.images ? json.images : [];


this.images = json.images || [];

paperboyo · 2017-11-01T19:43:47Z

Thanks so much!

Closing the lightbox doesn't remove the hash from the url (this is now fixed in this conversion)

Maybe this will finally fix #6144? Bravo! 🎆

Lightbox not working on articles (works now!)

Hmm, don't know anything about that, works for me (desktop, mobile).

Please note, I know of 3 current production bugs in lightbox.

Not counting the enhancements. And not counting the ones not recorded as issues (broken zoom, do-not-dare-swiping-up-on-Android-only-pros-know-how-to-not-have-addressbar-shown, lack of FullscreenAPI, close button not automatically appearing on last item, long captions breaking experience, not appearing for liveblog images, shut-up-paperboyo and many more 😆).

And integrating this for (for starters) disconnected members of a series 😉.

…ard JS

SiAdcock

LATM 👍

prout-bot · 2017-11-03T11:43:08Z

Seen on PROD (merged by @NataliaLKB 21 minutes and 48 seconds ago)

Check your changes on www.theguardian.com ✔️
Keep an eye on the deploy dashboard 📉

NataliaLKB requested review from GHaberis, gu-stav, SiAdcock, nicl and sndrs November 1, 2017 14:58

NataliaLKB force-pushed the es6-projects_common_modules_gallery_lightbox branch from 00bc5ad to 5d3e0ac Compare November 1, 2017 15:49

SiAdcock reviewed Nov 1, 2017

View reviewed changes

gu-stav reviewed Nov 1, 2017

View reviewed changes

NataliaLKB added 10 commits November 2, 2017 16:51

move projects/common/modules/gallery/lightbox.js from legacy to stand…

a1d4dad

…ard JS

transform projects/common/modules/gallery/lightbox.js to ES6 module

03ebf40

transform projects/common/modules/gallery/lightbox.js content to ES6

b31b5aa

passing validation, and the file works!

a9d960e

using config.get

e52845a

flow annotations

fee8cfd

remove default export and fix rebase

b77ff97

adding more flow types!

5e0c8eb

only exporting GalleryLightbox for tests

f4ea186

using a template literal instead of a concat string

ebd4e7b

NataliaLKB force-pushed the es6-projects_common_modules_gallery_lightbox branch from 83ae618 to ebd4e7b Compare November 2, 2017 16:52

SiAdcock approved these changes Nov 2, 2017

View reviewed changes

NataliaLKB merged commit 797bf03 into master Nov 3, 2017

NataliaLKB deleted the es6-projects_common_modules_gallery_lightbox branch November 3, 2017 11:21

prout-bot added Pending-on-PROD Seen-on-PROD and removed Pending-on-PROD labels Nov 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Es6 projects common modules gallery lightbox #18113

Es6 projects common modules gallery lightbox #18113

NataliaLKB commented Nov 1, 2017 •

edited

Loading

PRBuilds commented Nov 1, 2017 •

edited

Loading

SiAdcock commented Nov 1, 2017

SiAdcock Nov 1, 2017

NataliaLKB Nov 2, 2017

SiAdcock Nov 1, 2017

NataliaLKB Nov 2, 2017

gu-stav left a comment

gu-stav Nov 1, 2017

gu-stav Nov 1, 2017

gu-stav Nov 1, 2017

gu-stav Nov 1, 2017

NataliaLKB Nov 1, 2017

gu-stav Nov 1, 2017

gu-stav Nov 1, 2017

gu-stav Nov 1, 2017

gu-stav Nov 1, 2017

paperboyo commented Nov 1, 2017 •

edited

Loading

SiAdcock left a comment

prout-bot commented Nov 3, 2017


		let res;

		bean.on(document.body, 'click', '.js-gallerythumbs', e => {

Es6 projects common modules gallery lightbox #18113

Es6 projects common modules gallery lightbox #18113

Conversation

NataliaLKB commented Nov 1, 2017 • edited Loading

What does this change?

What is the value of this and can you measure success?

Does this affect other platforms - Amp, Apps, etc?

Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?

Screenshots

Tested in CODE?

PRBuilds commented Nov 1, 2017 • edited Loading

SiAdcock commented Nov 1, 2017

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

gu-stav 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

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

paperboyo commented Nov 1, 2017 • edited Loading

SiAdcock left a comment

Choose a reason for hiding this comment

prout-bot commented Nov 3, 2017

NataliaLKB commented Nov 1, 2017 •

edited

Loading

PRBuilds commented Nov 1, 2017 •

edited

Loading

paperboyo commented Nov 1, 2017 •

edited

Loading