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

Detect authors in RDFa in addition to Microdata #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

da2x
Copy link

@da2x da2x commented Feb 1, 2018

Case-insensitive matching.

@ThomasGreiner ThomasGreiner self-requested a review February 1, 2018 14:05
Copy link
Collaborator

@ThomasGreiner ThomasGreiner left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

Note that the author detection is currently only used to determine its potential impact and is therefore only limited to youtube.com. It'll come in handy later on though to have RDFa support.

@@ -9,10 +9,10 @@ function getAuthor()
let author = null;

// schema.org
let element = document.querySelector("[itemprop='author']");
let element = document.querySelector("[property='author' i],[itemprop='author' i]");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Coding style: Make sure to run npm run lint (see also README) so that it can let you know about any coding style issues it finds. In this case, for instance, it'd point out that the line is longer than the 80 characters we defined.

Copy link
Author

Choose a reason for hiding this comment

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

Make sure to run npm run lint (see also README)

This should be in CONTRIBUTING. It would pop-up whenever someone opens a pull-request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right and it's already on our list of to-dos to finish open sourcing the extension (along with creating a CHANGES.md file and moving existing documentation over to the GitHub Wiki). So I'm sorry about the lack of those for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I made an issue for this #11

Copy link
Collaborator

Choose a reason for hiding this comment

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

circleci should automatically run our tests (including the linters) for pull requests now, so you won't have to run it locally if you don't want to, or forget to. Once an update is pushed to this pull request it should run for this pr.

@@ -9,10 +9,10 @@ function getAuthor()
let author = null;

// schema.org
let element = document.querySelector("[itemprop='author']");
let element = document.querySelector("[property='author' i],[itemprop='author' i]");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Coding style: Please add a space before the second selector. Unfortunately, the linter doesn't catch that automatically.

Alos switch to case-insensitive matching.
Copy link
Collaborator

@ThomasGreiner ThomasGreiner left a comment

Choose a reason for hiding this comment

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

LGTM

Now the only thing that's left is to fill out our contributor's license agreement and send it back to us (e.g. via email to t.greiner@eyeo.com). That step is necessary for us to be able to use your code.

@ThomasGreiner
Copy link
Collaborator

@da2x I just quickly wanted to check back with you regarding the contributor's license agreement I mentioned in my previous comment. Mind sending that over so that we can merge your change?

@da2x
Copy link
Author

da2x commented Mar 12, 2018

I’ll not sign any agreement with any company over three lines of code. However, I’ll make it easy on you: I hereby waive all copyright or other legal claims on commit c77a514. (Independently archived copy.)

@owintwist
Copy link

owintwist commented Mar 12, 2018

You must add a CONTRIBUTING.md file on this repo with your Guidelines and CLA (or a link to)

These tools may be useful too :

@ThomasGreiner
Copy link
Collaborator

We're currently investigating various options together with our legal team on how we want to simplify this process including cla-assistant.io as well as DCOs. I've created #32 for making the according changes as soon as we got approval from them. Both of those methods seem to require the creation of a new pull request though.

I've also forwarded the comment with the waiver to legal and am waiting for their response. Note that it would only apply to this one contribution though, whereas the CLA applies to all contributions.

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.

4 participants