Gallery redesign WIP: Initial PR #12576

NataliaLKB · 2016-04-15T13:01:15Z

What does this change?

@zeftilldeath designed what galleries should look like, this is the initial PR implementing the design.

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

This will start as a 0% A/B test while we iterate. Next week the aim is to start rolling it out an audience

What is next?

These will all be done in future prs

Use picture element instead of srcset. We will refactor the scala img helper to reuse that, but outside this PR
Try to eliminate jank, at least where pictures are loading. Had a talk with @OliverJAsh yesterday about how to do this.
Add animation for when you scroll down (ask myself or @zeftilldeath for the video if you want to see)
On mobile, option to only see images and not captions.
Set a limit for caption length on both mobile and desktop
Fastly Change persistent cookie for optin so that people in the business can take a look without adding a header (https://github.com/guardian/fastly-edge-cache/pull/614)
Fix 100vh jumping on mobile across immersives. talked to @gtrufitt about this
MPU slot styling

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

No

Screenshots

Mobile:

Desktop:

Request for comment

@OliverJAsh

NataliaLKB · 2016-04-15T13:01:54Z

applications/app/views/fragments/newGalleryBody.scala.html

+
+@*
+
+TODO:


I left a lot of todos in the code for future iteration. Apologies in advance if it is confusing!

paperboyo · 2016-04-15T13:45:56Z

OMG!!111ONE! 😍

"Try to eliminate junk" & "Fix 100vh jumping on mobile" - maybe FulscreenAPI? Anyway: 🍻 for whomever succeeds!

Maybe also worth looking at the very small related #6144?

At the end, when all is done and everybody's drinking 🍾, we may think of joining galleries from the same series? Maybe? Like in this screenshot (or even without the disruptive endslate altogether): #5934 (comment)?

swipe...swipe...swipe...swipe...ad_impression...swipe...swipe...swipe...swipe...ad_impression...swipe...swipe...swipe...swipe...

jamesgorrie · 2016-04-15T14:15:10Z

@akash1810 Look at this beauty (not talking about @paperboyo )...

sammorrisdesign-zz · 2016-04-18T13:46:26Z

applications/app/views/fragments/newGalleryBody.scala.html

+
+
+
+


sammorrisdesign-zz · 2016-04-18T13:51:55Z

static/src/stylesheets/module/content/_gallery.scss

+            position: relative;
+            background-position: center center;
+            -webkit-background-size: cover;
+            background-size: cover;


Doesn't autoprefixer mean we don't have to add the -webkit rule?

Not sure, haven't written much scss in the project so far. Will remove now!

Removed. Thank you for reviewing! More comments, the better 👍

sammorrisdesign-zz · 2016-04-18T13:56:39Z

There are a bunch of sass whitespace and consistency things but I'm not going to be a pedant.

Only other thing I could think of is that the header is called immersive and the body is called new. Probably will be clearer to give it a single name.

Otherwise it's great that we're sorting these galleries out and it looks good. Probably will be better to get someone who knows what they're doing with scala for that side of things though.

NataliaLKB · 2016-04-18T14:01:28Z

@sammorrisdesign, do let me know about the sass whitespace and consistency things! Its useful to me, and easy to fix.

Regarding naming, once galleries are replaced, the new prefix can be removed.

sammorrisdesign-zz · 2016-04-18T14:04:57Z

But aren't the immersiveGalleryHead and newGalleryBody linked together? I'm just thinking in terms of someone coming to this fresh, they might not realise they are linked.

sammorrisdesign-zz · 2016-04-18T14:06:22Z

static/src/stylesheets/module/content/_gallery.scss

+        }
+    }
+
+


Double return inconsistent with single return below. I think we tend to have single lines after blocks. But should at least be consistent in same file.

NataliaLKB · 2016-04-18T14:23:39Z

But aren't the immersiveGalleryHeadTonal and newGalleryBody linked together? I'm just thinking in terms of someone coming to this fresh, they might not realise they are linked.

Thats fair. I will look first to seeing I can get rid of immersiveGalleryHeadTonal, and using the immersiveHeader.

johnduffell · 2016-04-18T14:43:28Z

applications/app/controllers/OptInController.scala

      case _ => NotFound
    }
  }

  val HTTPS = OptInFeature("https_opt_in")
  val Header = OptInFeature("new_header_opt_in")
+  val gallery = OptInFeature("gallery_redesign_opt_in")


just be careful about splitting the cache too much, otherwise the backend has to serve the same content twice

Its true, but we need to be able to test the value of this before we roll it out to everyone.

will you incorporate opting in into the a/b test somehow? otherwise it seems like opting in won't give you the new gallery :)

Do you mean in fastly? I have branch I pushed up waiting to go! Just want to get this out before I make another PR. If that isn't what you mean, come talk to me? Not sure I follow.

Note: This was discussed in person and the fastly change that makes this possible has already been made in a separate PR ready to go :)

OliverJAsh · 2016-04-20T15:20:09Z

applications/app/views/fragments/newGalleryBody.scala.html

+            Map("content--advertisement-feature" -> gallery.commercial.isAdvertisementFeature,
+                "paid-content--advertisement-feature" -> gallery.commercial.isAdvertisementFeature
+            ),
+            "content", "content--media", "content--gallery", "tonal", "new-gallery", "content--immersive", s"tonal--${toneClass(gallery)}"


Do we need content--immersive? Seems to me like it just creates more work for us as we need to override its styles later on.

As a note, Olly has separated this out into content--immersive for shared styles and content--immersive-article only used for immersive articles

paperboyo · 2016-04-20T22:07:42Z

Oh, apart from already cited #6144 it might be prudent to add #12215 & (at least half of) #12216 to the grand plan. There is also #9957, but my personal opinion is that currently zooming in is very broken on mobile, because it zooms in both the image and the UI (including captions, up to a point of no return). If we can’t make it so that only the image zooms in and the Lightbox’s UI stays put, there is not much point in having it at all, so no point extending it (“fixing”) for iOS...

… into gallery-redesign

desbo · 2016-04-25T10:43:29Z

common/app/views/fragments/header.scala.html

@@ -9,7 +9,7 @@
    @fragments.newHeader()
 } else {
    <header id="header"
-            class="l-header u-cf @if(page.metadata.hasSlimHeader) {l-header--is-slim l-header--no-navigation} js-header"
+            class="l-header u-cf @if(page.metadata.hasSlimHeader) {l-header--is-slim l-header--no-navigation} @if(mvt.ABGalleryRedesignVariant.isParticipating && page.metadata.contentType.toLowerCase == "gallery"){l-header--is-slim l-header--no-navigation l-header--new-gallery} js-header"


to avoid repetition of mvt.ABGalleryRedesignVariant.isParticipating && page.metadata.contentType.toLowerCase == "gallery", you might want to move it to a method inside the test

Changed this to shouldShow method which includes the check of whether you are on a gallery or not.

NataliaLKB · 2016-04-25T16:21:27Z

@desbo @sndrs would one of you be able to give another look? I believe all the comments have been addressed.

sndrs · 2016-04-26T11:26:15Z

static/src/stylesheets/module/content/_gallery.scss

+
+        // Overwriting media tone header so that you can see the image instead
+        .tonal__header {
+            background-color: rgb(0, 0, 0);


we don't have any other rgb colours in the codebase. for the sake of consistency, should they be #000000? they'll work fine so it's not a blocker.

Yep, actually changing them to $multimedia-support-5

sndrs · 2016-04-26T14:41:58Z

👍 do it!

johnduffell · 2016-04-26T14:56:56Z

haha @sndrs very appropriate!

dominickendrick · 2016-04-26T18:47:41Z

🚀 🎸 📷

NataliaLKB reviewed Apr 15, 2016
View reviewed changes

NataliaLKB force-pushed the gallery-redesign branch from 852eb23 to ad0e073 Compare April 15, 2016 13:02

NataliaLKB force-pushed the gallery-redesign branch from ac19d59 to e87a923 Compare April 15, 2016 15:23

sammorrisdesign-zz reviewed Apr 18, 2016
View reviewed changes

applications/app/views/fragments/newGalleryBody.scala.html

Copy link

sammorrisdesign-zz Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

NataliaLKB force-pushed the gallery-redesign branch from e87a923 to c9e82b5 Compare April 18, 2016 13:51

sammorrisdesign-zz reviewed Apr 18, 2016
View reviewed changes

NataliaLKB force-pushed the gallery-redesign branch from d5672a5 to 3ff0f3f Compare April 18, 2016 13:55

sammorrisdesign-zz reviewed Apr 18, 2016
View reviewed changes

NataliaLKB force-pushed the gallery-redesign branch 3 times, most recently from 32dfbd2 to dbc35b1 Compare April 18, 2016 14:17

johnduffell reviewed Apr 18, 2016
View reviewed changes

NataliaLKB force-pushed the gallery-redesign branch 2 times, most recently from fde0481 to 56fff0f Compare April 19, 2016 09:36

NataliaLKB added 4 commits April 19, 2016 15:12

adding new serverside 0% test

d482b72

adding new gallery template

27c88d8

adding slim header to gallery redesign

f0a387f

changing meta and share for new gallery

925dfd0

NataliaLKB force-pushed the gallery-redesign branch from e78d002 to 03e47d0 Compare April 20, 2016 12:30

styling changes

d714efd

NataliaLKB force-pushed the gallery-redesign branch from 03e47d0 to d714efd Compare April 20, 2016 12:41

OliverJAsh reviewed Apr 20, 2016
View reviewed changes

Fix aspect ratio at all breakpoints

ae40c55

OliverJAsh and others added 7 commits April 25, 2016 10:05

Merge branch 'master' into gallery-redesign

49d9fde

adding back lightbox link

0e06b06

using trail image instead of first image

28a2c5d

Separate immersive article styles into new class to reduce overrides

578e59e

Use white links in standfirst for all immersive content

d00b239

Remove redundant style

6b68ea3

Merge branch 'gallery-redesign' of https://github.com/guardian/frontend…

9a285a4

… into gallery-redesign

desbo reviewed Apr 25, 2016
View reviewed changes

NataliaLKB added 2 commits April 25, 2016 14:51

creating method shouldShow which includes the gallery check

b2f9f26

removing a couple of uneeded nested divs

271b576

NataliaLKB force-pushed the gallery-redesign branch from 6f7cec7 to 271b576 Compare April 25, 2016 14:48

NataliaLKB added 2 commits April 25, 2016 15:56

removing opacity 0, as not needed

892dbb0

making content__labels--gallery

585cbe9

Merge branch 'master' into gallery-redesign

fd37162

sndrs reviewed Apr 26, 2016
View reviewed changes

NataliaLKB added 2 commits April 26, 2016 14:23

Merge branch 'master' into gallery-redesign

1a5a9fa

more tweaks

b1803e6

NataliaLKB merged commit 113bb01 into master Apr 26, 2016

NataliaLKB deleted the gallery-redesign branch April 26, 2016 14:50

NataliaLKB mentioned this pull request May 31, 2016

Remove gallery switch #13134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gallery redesign WIP: Initial PR #12576

Gallery redesign WIP: Initial PR #12576

NataliaLKB commented Apr 15, 2016 •

edited

Loading

NataliaLKB Apr 15, 2016

paperboyo commented Apr 15, 2016

jamesgorrie commented Apr 15, 2016

sammorrisdesign-zz Apr 18, 2016

sammorrisdesign-zz Apr 18, 2016 •

edited

Loading

NataliaLKB Apr 18, 2016

NataliaLKB Apr 18, 2016

sammorrisdesign-zz commented Apr 18, 2016

NataliaLKB commented Apr 18, 2016

sammorrisdesign-zz commented Apr 18, 2016

sammorrisdesign-zz Apr 18, 2016

NataliaLKB commented Apr 18, 2016

johnduffell Apr 18, 2016 •

edited

Loading

NataliaLKB Apr 18, 2016

desbo Apr 20, 2016

NataliaLKB Apr 20, 2016

NataliaLKB Apr 26, 2016

OliverJAsh Apr 20, 2016

NataliaLKB Apr 25, 2016

paperboyo commented Apr 20, 2016

desbo Apr 25, 2016 •

edited

Loading

NataliaLKB Apr 25, 2016

NataliaLKB commented Apr 25, 2016

sndrs Apr 26, 2016

NataliaLKB Apr 26, 2016

sndrs commented Apr 26, 2016

johnduffell commented Apr 26, 2016 •

edited

Loading

dominickendrick commented Apr 26, 2016


		@*

		TODO:

Gallery redesign WIP: Initial PR #12576

Gallery redesign WIP: Initial PR #12576

Conversation

NataliaLKB commented Apr 15, 2016 • edited Loading

What does this change?

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

What is next?

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

Screenshots

Request for comment

Choose a reason for hiding this comment

paperboyo commented Apr 15, 2016

jamesgorrie commented Apr 15, 2016

Choose a reason for hiding this comment

sammorrisdesign-zz Apr 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sammorrisdesign-zz commented Apr 18, 2016

NataliaLKB commented Apr 18, 2016

sammorrisdesign-zz commented Apr 18, 2016

Choose a reason for hiding this comment

NataliaLKB commented Apr 18, 2016

johnduffell Apr 18, 2016 • 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

paperboyo commented Apr 20, 2016

desbo Apr 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NataliaLKB commented Apr 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sndrs commented Apr 26, 2016

johnduffell commented Apr 26, 2016 • edited Loading

dominickendrick commented Apr 26, 2016

NataliaLKB commented Apr 15, 2016 •

edited

Loading

sammorrisdesign-zz Apr 18, 2016 •

edited

Loading

johnduffell Apr 18, 2016 •

edited

Loading

desbo Apr 25, 2016 •

edited

Loading

johnduffell commented Apr 26, 2016 •

edited

Loading