Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

add twitter and open graph cards #428

Merged
merged 2 commits into from Aug 11, 2017
Merged

add twitter and open graph cards #428

merged 2 commits into from Aug 11, 2017

Conversation

johngruen
Copy link
Contributor

Fixes #153

Note. I didn't localize b/c i'm not sure it's possible to localize content attrs.

<meta name="twitter:card" content="summary"/>
<meta property="og:image" content="/resources/send-fb.jpg"/>
<meta name="twitter:image" content="/resources/send-twitter.jpg"/>
<meta property="og:url" content="https://send.firefox.com"/>
Copy link
Collaborator

@pdehaan pdehaan Aug 4, 2017

Choose a reason for hiding this comment

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

These last 3 seem almost questionable...

  1. Should we be pulling og:url from some environment? Like check conf.env and set this to dev/stage/prod as appropriate? I'm not 100% certain what the og:url is used for.
  2. For og:image and twitter:image, I thought those needed to be fully qualified URLs. I vaguely recall us struggling with this on Test Pilot site. Ref: mozilla/testpilot /frontend/tasks/pages.js:16-19

Although hard coding prod URLs was causing issues for QA since we cannot test the Twitter and Facebook cards until you push it to production, which isn't always ideal since it's too late at that point and we'd need more production pushes in order to verify tweaks.

Duplication of title/description strings doesn't seem DRY either. Not sure if we want to pass those into Handlebars as helpers (like we did for availableLanguages), not sure if that'd help w/ localization support or not since this would be server-side.


UPDATE: This was what I was kvetching about re: DRY strings:

diff --git a/server/server.js b/server/server.js
index 8415f06..ee32cd0 100644
--- a/server/server.js
+++ b/server/server.js
@@ -11,6 +11,7 @@ const Raven = require('raven');
 const crypto = require('crypto');
 const fs = require('fs');
 const version = require('../public/version.json');

 if (conf.sentry_dsn) {
   Raven.config(conf.sentry_dsn).install();
@@ -46,7 +47,9 @@ app.engine(
     partialsDir: 'views/partials/',
     helpers: {
       availableLanguages,
-      l10nDev: conf.l10n_dev
+      l10nDev: conf.l10n_dev,
+      title: 'Firefox Send',
+      description: 'Encrypt and send files with a link that automatically expires to ensure your important documents don’t stay online forever.'
     }
   })
 );


diff --git a/views/layouts/main.handlebars b/views/layouts/main.handlebars
index 15a1fdc..b63fb3b 100644
--- a/views/layouts/main.handlebars
+++ b/views/layouts/main.handlebars
@@ -6,7 +6,17 @@
   <meta name="defaultLanguage" content="en-US">
   <meta name="availableLanguages" content="{{availableLanguages}}">

-  <title>Firefox Send</title>
+  <meta name="description" content="{{description}}"/>
+  <meta name="twitter:card" content="summary"/>
+  <meta name="twitter:description" content="{{description}}"/>
+  <meta name="twitter:image" content="https://send.firefox.com/resources/send-twitter.jpg"/>
+  <meta name="twitter:title" content="{{title}}"/>
+  <meta property="og:description" content="{{description}}"/>
+  <meta property="og:image" content="https://send.firefox.com/resources/send-fb.jpg"/>
+  <meta property="og:title" content="{{title}}"/>
+  <meta property="og:url" content="https://send.firefox.com"/>
+
+  <title>{{title}}</title>

   <link rel="stylesheet" type="text/css" href="/main.css" />
   {{#if fira}}

Copy link
Collaborator

@pdehaan pdehaan Aug 4, 2017

Choose a reason for hiding this comment

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

@dannycoates Does the Express server know whether it's serving as "https://send.firefox.com", "send.stage.mozaws.net", or "send.dev.mozaws.net"?

I think our only supported NODE_ENV values are "production", "development", and "test" (per "/server/config.js:35-39"), so not sure I could to detect a difference between dev/stage:

send/server/config.js

Lines 35 to 39 in 80db158

env: {
format: ['production', 'development', 'test'],
default: 'development',
env: 'NODE_ENV'
},

@pdehaan
Copy link
Collaborator

pdehaan commented Aug 4, 2017

The weird thing is that these strings are kinda already localized in the /public/locales/*/send.ftl files (but then we'd need to figure out server side translations):

title = Firefox Send
uploadPageExplainer = Send files through a safe, private, and encrypted link that automatically expires to ensure your stuff does not remain online forever.

Also our meta tags and site copy vary by just a few words... Super ultra nit, but possibly worth pointing out. FWIW, I like your new copy better (mainly because I don't care for the word "stuff"), but changing the FTL files to match new copy would have a l10n penalty:

Current uploadPageExplainer FTL text:

"Send files through a safe, private, and encrypted link that automatically expires to ensure your stuff does not remain online forever."

Proposed <meta description=""> text:

"Encrypt and send files with a link that automatically expires to ensure your important documents don’t stay online forever."

@johngruen
Copy link
Contributor Author

@pdehaan i used the copy from the test pilot site which, for reasons i don't fully understand, makes legal more happy.

server/server.js Outdated
@@ -38,6 +38,16 @@ function prodLangs() {

const availableLanguages = conf.l10n_dev ? allLangs() : prodLangs();

const envURL = (env) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using this to set the URL so this can be tested on dev environments. Not super familiar with convict, but maybe this would be better done in config

server/server.js Outdated
l10nDev: conf.l10n_dev,
envURL: envURL(conf.env),
title: 'Firefox Send',
description: 'Encrypt and send files with a link that automatically expires to ensure your important documents don’t stay online forever.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, this doesn't get localized, maybe that's not possible...

@@ -7,7 +7,17 @@
<meta name="defaultLanguage" content="en-US">
<meta name="availableLanguages" content="{{availableLanguages}}">

<title>Firefox Send</title>
<meta property="og:title" content="Firefox Send"/>
<meta name="twitter:title" content="Firefox Send"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: It may be nice to use {{title}} here too for maximum efficiency. But merging is fun too and that could be micro-optimized later.

server/server.js Outdated
const envURL = (env) => {
switch (env) {
case 'test':
return 'https://send.stage.mozaws.net';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced this is a thing.
I have a feeling that stage would use an env of "production". Not sure what [if anything] uses the "test" NODE_ENV, unless Circle-CI somehow sets it to that, but I strangely kind of doubt it.

I don't know how to tell the detect whether we're running on stage vs prod on the server.

@dannycoates
Copy link
Contributor

I extracted the url to a env var. Here's the issue to set it https://github.com/mozilla-services/cloudops-deployment/issues/1018

@dannycoates dannycoates merged commit e142d76 into master Aug 11, 2017
@dannycoates dannycoates deleted the add-twitter-og-cards branch August 14, 2017 00:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants