Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - Email front flagship #20465

Merged
merged 8 commits into from Oct 25, 2018
Merged

WIP - Email front flagship #20465

merged 8 commits into from Oct 25, 2018

Conversation

tomrf1
Copy link
Member

@tomrf1 tomrf1 commented Oct 4, 2018

What does this change?

The new flagship podcast will have a permanent weekday spot in the email front.
The container will be backfilled from the tag series, and the container will display a single card for the latest episode.
The image is hardcoded, not taken from the content. It otherwise looks like a normal medium container.

It relies on the name of the container being constant. This is brittle, but the alternative is making this configurable in the fronts tool and reflected in the model (which is a lot of work for a single special case).

Screenshots

(with football weekly for now)
picture 25

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

Checklist

Does this affect other platforms?

  • AMP
  • Apps
  • Other (please specify)

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

  • No
  • Yes (please give details)

Does this change break ad-free?

  • No
  • It did, but tests caught it and I fixed it
  • It did, but there was no test coverage so I added that then fixed it

Accessibility test checklist

Tested

  • Locally
  • On CODE (optional)

@faciaCard(classesForCard(card, withImage = true)) {
@row(Seq("no-pad")){
<a class="fc-link" href="@card.header.url.hrefWithRel">
<img class="full-width" src="https://i.guim.co.uk/img/media/60478bdc559fa72fe4fe0ac09636f78df9f3cdb9/3_170_3910_2345/master/3910.jpg?width=500&amp;quality=60&amp;fit=max&amp;s=a31b2353a7f08f4417006bda4a77acf5" alt="@card.header.headline">
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO - use the correct asset

@tomrf1 tomrf1 changed the title Email front flagship WIP - Email front flagship Oct 4, 2018
@@ -286,6 +315,7 @@
}

@layout.CollectionEmail.fromPressedPage(page).collections.zipWithIndex.map {
case (collection: EmailContentContainer, index) if collection.displayName == "Flagship" => { @renderFlagshipContainer(collection) }
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO - correct container name

Copy link
Member Author

Choose a reason for hiding this comment

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

Use the values here #20473

@PRBuilds
Copy link

PRBuilds commented Oct 4, 2018

PRbuilds results:

Screenshots
wide.pngdesktop.pngtablet.pngmobile.png

💚 Exceptions
thrown-exceptions.js

💚 A11y validation
a11y-report.txt

💚 Microdata Validation
microdata.txt

Apache Benchmark Load Testing
loadtesting.txt

LightHouse Reporting
lighthouse.html

--automated message

@mattoh1
Copy link

mattoh1 commented Oct 25, 2018

👍 🥇

FlagshipEmailContainerSwitch.isSwitchedOn &&
now.isAfter(GoLiveDateTime) &&
now.getDayOfWeek != SATURDAY && now.getDayOfWeek != SUNDAY &&
!(now.getDayOfWeek == MONDAY && now.getHourOfDay < 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

My head is spinning 🤯 is this 4am, London time? It might be worth adding a small comment to clarify these two lines.

@@ -206,6 +207,30 @@
}
}

@renderFlagshipContainer(collection: EmailContentContainer) = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Much cleaner 😍

Copy link
Contributor

@regiskuckaertz regiskuckaertz left a comment

Choose a reason for hiding this comment

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

YASS!

@mattoh1
Copy link

mattoh1 commented Oct 25, 2018

FYI... email newsletter send times...

UK 5am local time
AU 12pm Sydney time (currently 2am UK but 1am as of Sunday)
US ~8am NY time, give or take 30 mins (currently 1pm UK, but 12pm as of Sunday, and then back to 1pm the Sunday after that)

We want to pick these up weekdays only, no container on weekends in emails newsletters
cc: @tomrf1 @regiskuckaertz

@mattoh1
Copy link

mattoh1 commented Oct 25, 2018

Additionally..
Container desc: Podcast
Kicker: Today in Focus
Kicker and speaker colour: red like series page example?

@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @tomrf1 14 minutes and 17 seconds ago)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants