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

Add support for Rewards Github publishers #20

Merged
merged 2 commits into from
Oct 13, 2020
Merged

Add support for Rewards Github publishers #20

merged 2 commits into from
Oct 13, 2020

Conversation

emerick
Copy link
Contributor

@emerick emerick commented Oct 6, 2020

Associated brave-core PR: brave/brave-core#6789

Adds support for Github-based publisher tips via a Greaselion script. The script reacts to URL navigations and state changes by sending the associated publisher info to the Rewards extension, allowing tips to be supported via the Rewards panel and an injected inline tip button in the user's Github pages.

@emerick emerick self-assigned this Oct 6, 2020
@emerick emerick force-pushed the rewards-github branch 2 times, most recently from e00e787 to 34fc02e Compare October 6, 2020 17:59
@emerick emerick force-pushed the rewards-github branch 2 times, most recently from c0b0725 to dcfb025 Compare October 7, 2020 13:33
@emerick emerick requested a review from NejcZdovc October 7, 2020 13:34
tipButton.appendChild(tipIconContainer)

// Create the tip icon
const tipIcon = document.createElement('span')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible (and if so, worth it) to generalize this DOM creation operation across the sites (so that it would live in the "common" folder)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll enter a new issue for that. This function is particularly funny because it was originally copied from the Twitter content script. I guess the container layout also happens to work for Github, but it gives the false impression that Github is using this same layout/naming when it actually isn't.

return tipAction
}

const getCommentMetaData = (elem: Element) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be an async function, with the return Project.reject(... replaced with throw ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

parent.insertBefore(tipAction, end)
}

const getCommitLinksMetaData = (elem: Element) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably be an async function as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

parent.appendChild(tipAction)
}

const getStarringContainerMetaData = (elem: Element) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably be an async function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

parent.insertBefore(tipAction, elements[0])
}

const getPageHeadMetaData = (elem: Element) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably be an async function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

const configureTipActions = () => {
clearTimeout(configureTipActionsTimeout)

const tippingFunctions = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think it would be more clear to not use this data structure and simply call configureTipAction however many times is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fixed.

}

const handleOnUpdatedTab = (changeInfo: any) => {
if (!changeInfo || (!changeInfo.url && changeInfo.status !== 'complete')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little unclear on the filtering here - when do want the action below to run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was a little odd. I was receiving multiple update notifications, I'm assuming it's related to Github using the history API as this bug seems to suggest: https://bugs.chromium.org/p/chromium/issues/detail?id=465709.

In order to work around that, I look for a changInfo with a URL or a status of complete and then store the location if it doesn't match what we already have stored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good - mind adding a short comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, added.

})
}

const getMediaMetaData = (screenName: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably be an async function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

return Promise.reject(new Error('Invalid profile api url'))
}

return fetch(profileApiUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use an async function, you can await fetch(....) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

})
})
.catch((error: any) => {
throw new Error(`Fetch request failed: ${error}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Throwing from a catch handler will just result in a new rejected promise. You probably don't need the catch handler here (unless you want to add extra error context).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yep, fixed.

Greaselion.json Outdated
@@ -1,4 +1,16 @@
[
{
"urls": [
"https://*.github.com/*"
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to support sub domains?

I would guess we can do something like this

"https://github.com/*",
"https://www.github.com/*"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that we need to support at least gist.github.com, but I could just manually list that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made that change.

@emerick emerick force-pushed the rewards-github branch 2 times, most recently from d0e5b5f to 716e73f Compare October 7, 2020 15:54
throw new Error('Invalid profile api url')
}

return await fetch(profileApiUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use await to get rid of all of the then callbacks, if you like:

const response = await fetch(profileApiUrl)
if (!response.ok) {
  throw new Error(`Profile API request failed: ${response.statusText} (${response.status})`)
}
const data = await response.json()
return ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I definitely do want to get rid of that - thanks! Fixed.

@emerick emerick merged commit 4e31d67 into master Oct 13, 2020
@emerick emerick deleted the rewards-github branch October 13, 2020 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants