✨Apester Media (extension): Added meta tags extraction #17305

OmriKeret · 2018-08-06T06:20:55Z

Added the ability to extract article keywords & title

Due to increasing number of adapters of the AMP framework among publishers (as a complete desktop + mobile solution)we added the ability to extract article's meta keyword and title

Feature: extract article meta keywords when using the playlist embed code
Feature: extract article title when json ld exists on the page
Fix: loader kept on hanging when there was no content to display

… title for better contextual matching

ghost · 2018-08-06T07:15:03Z

This pull request introduces 1 alert when merging 1833d22 into 9cee51b - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

ghost · 2018-08-06T12:21:29Z

This pull request introduces 1 alert when merging 3832deb into 9cee51b - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

ghost · 2018-08-06T14:17:16Z

This pull request introduces 1 alert when merging 875bcc8 into 9cee51b - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

OmriKeret · 2018-08-06T14:17:17Z

Hi @aghassemi,
We have an increasing number of adaptors who serve amphtml to their entire consumer base (not just through google cdn) and they use meta-keywords or jsonLd to describe their site.

We extract part of it's content to in order to improve our automatic contextual matching (which is the common implementation of the Apester units).

In addition I've updated the Apester loader and fixed an issue where we didn't remove the component when there was no content.
Can you review?

Thanks!

ghost · 2018-08-06T15:03:24Z

This pull request introduces 1 alert when merging 23725e5 into 9cee51b - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

ghost · 2018-08-06T15:49:35Z

This pull request introduces 1 alert when merging ff3ad24 into 9c05a22 - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

codecov-io · 2018-08-06T15:57:47Z

Codecov Report

Merging #17305 into master will increase coverage by <.01%.
The diff coverage is 89.55%.

@@            Coverage Diff             @@
##           master   #17305      +/-   ##
==========================================
+ Coverage   77.44%   77.44%   +<.01%     
==========================================
  Files         565      565              
  Lines       41422    41472      +50     
==========================================
+ Hits        32079    32118      +39     
- Misses       9343     9354      +11

Flag	Coverage Δ
#integration_tests	`36.05% <ø> (-0.01%)`	⬇️
#unit_tests	`76.5% <89.55%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cee51b...9682bb3. Read the comment docs.

ghost · 2018-08-06T17:00:06Z

This pull request introduces 1 alert when merging b3a8c80 into f381235 - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

ghost · 2018-08-06T18:18:48Z

This pull request introduces 1 alert when merging c01fdd6 into 86ae732 - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

ghost · 2018-08-07T08:23:35Z

This pull request introduces 1 alert when merging 48253b5 into 7259114 - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

… directly

ghost · 2018-08-07T09:40:37Z

This pull request introduces 1 alert when merging c1bfb60 into 7259114 - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

ghost · 2018-08-07T13:12:56Z

This pull request introduces 1 alert when merging ced2e70 into 7259114 - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

ghost · 2018-08-07T13:46:50Z

This pull request introduces 1 alert when merging 488892e into 4e59eb5 - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

ghost · 2018-08-07T15:41:30Z

This pull request introduces 1 alert when merging 9682bb3 into 59bd094 - view on LGTM.com

new alerts:

1 for Missing space in string concatenation

Comment posted by LGTM.com

aghassemi · 2018-08-13T21:48:01Z

build-system/tasks/karma.conf.js

@@ -228,7 +228,7 @@ module.exports = {
      timeout: process.env.TRAVIS ? 10000 : 2000,
    },
    captureConsole: true,
-    verboseLogging: false,
+    verboseLogging: true,


aghassemi · 2018-08-13T21:49:00Z

extensions/amp-apester-media/0.1/amp-apester-media.js

@@ -137,7 +140,7 @@ class AmpApesterMedia extends AMP.BaseElement {
        (this.random_ = this.element.getAttribute(
            'data-apester-channel-token'
        )),
-        'Either the data-apester-media-id or the data-apester-channel-token ' +
+        'Either the data-apester-media-id or the data-apester-channel-token' +


add space at the end of sentence (broken concatenation)

aghassemi · 2018-08-13T21:52:36Z

extensions/amp-apester-media/0.1/utils.js

+ * @return {!Array<string>}
+ */
+export function extratctTitle(root) {
+  const scriptTags = root.querySelectorAll(


You can use toArray from type.js here and then no longer need to do Array.prototype.map

… to types/toArray

…phtml into feature/contextual-random-unit

OmriKeret · 2018-08-14T07:43:35Z

Hi @aghassemi, Thanks for reviewing!
I changed the PR according to your comments.
Not sure why Travis fails since I merged with master.

Could you please review it again?
Thanks!

aghassemi · 2018-08-14T18:27:22Z

@OmriKeret merged.

)

OmriKeret added 2 commits August 1, 2018 15:44

feature(amp-apester-media): added collection of meta tags and article…

bc1b244

… title for better contextual matching

tests(apester media): added tests

f54b484

googlebot added the cla: yes label Aug 6, 2018

OmriKeret added 2 commits August 6, 2018 09:24

fix(amp apester media utils): renamed variable jsonDl to jsonLd

16c5591

style(amp apester media): linting

1833d22

OmriKeret added 2 commits August 6, 2018 14:54

feature(amp apester media): switch old loader with new loader

890bb21

style(amp apester media): lint

3832deb

OmriKeret added 2 commits August 6, 2018 16:49

fix(apester media): unlayout when there is no content

1f63ea1

style(amp apester media): lint

875bcc8

fix(amp apester media): fixed travis erros

23725e5

OmriKeret added 2 commits August 6, 2018 18:20

fix(apester): fixed check types

64ebb69

fix(apester media): fixed types

ff3ad24

OmriKeret added 2 commits August 6, 2018 19:32

reverted jsonld accidentale change

5cac071

reverted jsonld accidentale change

b3a8c80

aghassemi self-requested a review August 6, 2018 16:38

fix(amp apester media): fixed types

c01fdd6

fix(amp apester medai): changed types

48253b5

fix(amp apester media): pass util extract tags function the root node…

c1bfb60

… directly

fix(apester media): fixed tests

ced2e70

fix(amp apester media): removed console log

488892e

remove comments

9682bb3

aghassemi reviewed Aug 13, 2018

View reviewed changes

OmriKeret added 6 commits August 14, 2018 09:02

chore(karma.cofnf: reverted change to verboseLogging flag

282f8ce

fix(apester-media): fied broken concatenation

487d8e0

refactor(amp-apester-media-utils): changed the use of Array.proto.map…

bff7f7b

… to types/toArray

Merge branch 'master' into feature/contextual-random-unit

99ce98d

Merge branch 'feature/contextual-random-unit' of github.com:Qmerce/am…

c87803f

…phtml into feature/contextual-random-unit

lint(amp-apester-media): fixed linting

ab2d739

aghassemi approved these changes Aug 14, 2018

View reviewed changes

aghassemi merged commit 58ff9ae into ampproject:master Aug 14, 2018

OmriKeret deleted the feature/contextual-random-unit branch August 15, 2018 07:20

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018

✨Apester Media (extension): Added meta tags extraction (ampproject#17305

49ec53b

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Apester Media (extension): Added meta tags extraction #17305

✨Apester Media (extension): Added meta tags extraction #17305

OmriKeret commented Aug 6, 2018 •

edited

ghost commented Aug 6, 2018

ghost commented Aug 6, 2018

ghost commented Aug 6, 2018

OmriKeret commented Aug 6, 2018

ghost commented Aug 6, 2018

ghost commented Aug 6, 2018

codecov-io commented Aug 6, 2018 •

edited

ghost commented Aug 6, 2018

ghost commented Aug 6, 2018

ghost commented Aug 7, 2018

ghost commented Aug 7, 2018

ghost commented Aug 7, 2018

ghost commented Aug 7, 2018

ghost commented Aug 7, 2018

aghassemi Aug 13, 2018

aghassemi Aug 13, 2018

aghassemi Aug 13, 2018

OmriKeret commented Aug 14, 2018

aghassemi commented Aug 14, 2018

✨Apester Media (extension): Added meta tags extraction #17305

✨Apester Media (extension): Added meta tags extraction #17305

Conversation

OmriKeret commented Aug 6, 2018 • edited

Added the ability to extract article keywords & title

ghost commented Aug 6, 2018

ghost commented Aug 6, 2018

ghost commented Aug 6, 2018

OmriKeret commented Aug 6, 2018

ghost commented Aug 6, 2018

ghost commented Aug 6, 2018

codecov-io commented Aug 6, 2018 • edited

Codecov Report

ghost commented Aug 6, 2018

ghost commented Aug 6, 2018

ghost commented Aug 7, 2018

ghost commented Aug 7, 2018

ghost commented Aug 7, 2018

ghost commented Aug 7, 2018

ghost commented Aug 7, 2018

aghassemi Aug 13, 2018

Choose a reason for hiding this comment

aghassemi Aug 13, 2018

Choose a reason for hiding this comment

aghassemi Aug 13, 2018

Choose a reason for hiding this comment

OmriKeret commented Aug 14, 2018

aghassemi commented Aug 14, 2018

OmriKeret commented Aug 6, 2018 •

edited

codecov-io commented Aug 6, 2018 •

edited