Normalize Referrer classes across devices #1270

jridgewell · 2015-12-30T19:24:52Z

For now, these include Facebook, Twitter, and Pinterest with amp-ua-${SERVICE}.

camelburrito · 2015-12-30T19:38:00Z

extensions/amp-dynamic-css-classes/0.1/test/test-runtime-classes.js

-describe('dynamic classes are inserted at runtime', () => {
-  let documentElement, win;
-  beforeEach(() => {
+describe.only('dynamic classes are inserted at runtime', () => {


remove the only

jridgewell · 2015-12-30T19:42:48Z

Assigning to @cramforce, need to know what other apps we want to support.

camelburrito · 2015-12-30T19:44:57Z

extensions/amp-dynamic-css-classes/amp-dynamic-css-classes.md

+
+**amp-ua-***
+
+A User Agent class will be added when we detect that the document is


document where the class would be added. (body/documentElement? )

Is https://github.com/ampproject/amphtml/blob/master/extensions/amp-dynamic-css-classes/amp-dynamic-css-classes.md#behavior not enough?

camelburrito · 2015-12-30T19:46:30Z

LGTM

dvoytenko · 2015-12-30T19:47:40Z

My main question is whether the distinction between referrer and UA is apparent enough. A lot of times, I assume, these could mean the same thing, especially when the document is shown in the iframe. I think it's critical we clarify the distinction between the two or remove it entirely.

dvoytenko · 2015-12-30T19:48:24Z

src/platform.js

@@ -27,8 +27,8 @@ export class Platform {
   * @param {!Window} win
   */
  constructor(win) {
-    /** @const {!Window} */
-    this.win = win;
+    /** @const {string} */


It's not a string, right?

Whoops, fixed.

cramforce · 2016-01-03T17:50:06Z

extensions/amp-dynamic-css-classes/0.1/amp-dynamic-css-classes.js

+
+function addUserAgentClasses(win) {
+  const classes = ['Facebook', 'Twitter', 'Pinterest'].filter(service => {
+    return platform[`is${service}`]();


This pattern does not work with closure compiler advanced optimizations.

jridgewell · 2016-01-12T23:45:41Z

Updated code, please take another look. Will update the docs tomorrow.

cramforce · 2016-01-13T00:03:29Z

Code looks good.

dvoytenko · 2016-01-13T00:40:18Z

extensions/amp-dynamic-css-classes/0.1/amp-dynamic-css-classes.js

+
+  // Normalize t.co names to twitter.com
+  if (referrer === 't.co') {
+    referrers = referrers.concat(referrers_('twitter.com'));


So, we will have both, "t-co" and "twitter-com"?

Yup, this will include the both of them. Per my talk with @cramforce yesterday:

Should we normalize t.co links to twitter.com?

That would be cool, yeah
Because I want them to get rid of t.co
And if they did we likely would put the opposite hack on our side

So it seems to me we should allow people to use t.co if they're aware of twitter's short links, or twitter.com if they're not.

I'd actually prefer to use twitter.com exclusively.

Don't you think that'll be confusing for the people who are expecting t.co links?

They can always just see which classes are present in the cases they are testing.

cramforce · 2016-01-13T19:55:34Z

extensions/amp-dynamic-css-classes/0.1/test/test-runtime-classes.js

+    it('should normalize twitter shortlinks to twitter', () => {
+      return setup(true, '', tcoReferrer).then(() => {
+        expect(documentElement).to.have.class('amp-referrer-co');
+        expect(documentElement).to.have.class('amp-referrer-t-co');


Why is this still being added?

Forgot to remove it, sorry.

cramforce · 2016-01-13T19:56:18Z

Do you still have it on your radar to make this extension block the page from unhiding the body before it ran?

cramforce · 2016-01-13T20:05:37Z

LGTM

jridgewell · 2016-01-13T20:18:30Z

Added #1415 to track the FOUC issue.

- We normalize `t.co` referrers to `twitter.com` - We add a `www.pinterest.com` referrer when inside the Android Pinterest app.

Normalize Referrer classes across devices

jridgewell force-pushed the dynamic-user-agent-classes branch from bcb0f01 to 7b8fba5 Compare December 30, 2015 19:32

jridgewell added the NEEDS REVIEW label Dec 30, 2015

jridgewell assigned camelburrito Dec 30, 2015

jridgewell force-pushed the dynamic-user-agent-classes branch from 7b8fba5 to 6150942 Compare December 30, 2015 19:34

camelburrito reviewed Dec 30, 2015
View reviewed changes

jridgewell force-pushed the dynamic-user-agent-classes branch from b6d8179 to 2ea67d6 Compare December 30, 2015 19:41

jridgewell assigned cramforce and unassigned camelburrito Dec 30, 2015

camelburrito reviewed Dec 30, 2015
View reviewed changes

dvoytenko reviewed Dec 30, 2015
View reviewed changes

jridgewell force-pushed the dynamic-user-agent-classes branch 2 times, most recently from 5ea0456 to a9ddbc6 Compare December 30, 2015 19:50

cramforce reviewed Jan 3, 2016
View reviewed changes

jridgewell force-pushed the dynamic-user-agent-classes branch from a9ddbc6 to ef823f3 Compare January 12, 2016 23:44

jridgewell changed the title ~~Include Service identifier dynamic classes~~ Normalize Referrer classes across devices Jan 12, 2016

dvoytenko reviewed Jan 13, 2016
View reviewed changes

jridgewell force-pushed the dynamic-user-agent-classes branch 3 times, most recently from 5f5245c to 9bb543a Compare January 13, 2016 19:46

cramforce reviewed Jan 13, 2016
View reviewed changes

jridgewell force-pushed the dynamic-user-agent-classes branch from 9bb543a to 758be54 Compare January 13, 2016 19:57

cramforce added LGTM and removed NEEDS REVIEW labels Jan 13, 2016

jridgewell force-pushed the dynamic-user-agent-classes branch from 758be54 to 19d4dc7 Compare January 13, 2016 20:54

Normalize certain referrers across devices

8e71f6f

- We normalize `t.co` referrers to `twitter.com` - We add a `www.pinterest.com` referrer when inside the Android Pinterest app.

jridgewell force-pushed the dynamic-user-agent-classes branch from 19d4dc7 to 8e71f6f Compare January 13, 2016 21:33

jridgewell added a commit that referenced this pull request Jan 13, 2016

Merge pull request #1270 from jridgewell/dynamic-user-agent-classes

3f6282a

Normalize Referrer classes across devices

jridgewell merged commit 3f6282a into ampproject:master Jan 13, 2016

jridgewell deleted the dynamic-user-agent-classes branch January 13, 2016 22:16

elijahsoria mentioned this pull request Sep 24, 2020

SwG Release 0.1.22.121 #30383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize Referrer classes across devices #1270

Normalize Referrer classes across devices #1270

jridgewell commented Dec 30, 2015

camelburrito Dec 30, 2015

jridgewell Dec 30, 2015

jridgewell commented Dec 30, 2015

camelburrito Dec 30, 2015

jridgewell Dec 30, 2015

camelburrito commented Dec 30, 2015

dvoytenko commented Dec 30, 2015

dvoytenko Dec 30, 2015

jridgewell Dec 30, 2015

cramforce Jan 3, 2016

jridgewell commented Jan 12, 2016

cramforce commented Jan 13, 2016

dvoytenko Jan 13, 2016

jridgewell Jan 13, 2016

cramforce Jan 13, 2016

jridgewell Jan 13, 2016

cramforce Jan 13, 2016

jridgewell Jan 13, 2016

cramforce Jan 13, 2016

jridgewell Jan 13, 2016

cramforce commented Jan 13, 2016

cramforce commented Jan 13, 2016

jridgewell commented Jan 13, 2016


		amp-ua-*

		A User Agent class will be added when we detect that the document is

Normalize Referrer classes across devices #1270

Normalize Referrer classes across devices #1270

Conversation

jridgewell commented Dec 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Dec 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelburrito commented Dec 30, 2015

dvoytenko commented Dec 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Jan 12, 2016

cramforce commented Jan 13, 2016

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

cramforce commented Jan 13, 2016

cramforce commented Jan 13, 2016

jridgewell commented Jan 13, 2016