-
Notifications
You must be signed in to change notification settings - Fork 975
Add sharing current page menu items (Email, Twitter, Facebook...) #8517
Conversation
Fix #3121 Auditors: @bsclifton
tabs.js is an api file so shouldn't know about actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback left- follow up commits always appreciated. LGTM! 😄
const {makeImmutable} = require('../../common/state/immutableUtil') | ||
const {simpleShareActiveTab} = require('../share') | ||
|
||
const shareReducer = (state, action) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought on this one- what other types of sharing do we think we'll have? Could this possibly handle things like Submit Feedback
in the Help Menu, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submit feedback would make sense to be moved into here. In the future I was thinking new tab stats sharing from the new tab page would go into here as well.
googlePlus: 'https://plus.google.com/share?url={url}', | ||
linkedIn: 'https://www.linkedin.com/shareArticle?url={url}&title={title}', | ||
buffer: 'https://buffer.com/add?text={title}&url={url}', | ||
digg: 'http://digg.com/submit?url={url}&title={title}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does digg not offer an https URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's an oversight but we aren't using it because Brad asked not to, I'll change it in case we do use it in the future though.
@@ -80,6 +80,14 @@ newSessionTab=New Session Tab | |||
newWindow=New Window | |||
reopenLastClosedTab=Reopen Last Closed Tab | |||
print=Print… | |||
emailPageLink=Email Page Link… | |||
tweetPageLink=Tweet Page… | |||
facebookPageLink=Share on Facebook… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these would make more sense as the generic template, like "Share on {{siteName}}" and the the l10n library could have the service name input as args. This would prevent translators from possibly changing the usage of the name / brand / platform for the share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted a follow up task marked as good-first-bug for this review comment:
#8604
const url = templateUrl | ||
.replace('{url}', encodedUrl) | ||
.replace('{title}', encodedTitle) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ on the encodeURLComponent calls 😄
Fix #3121
Auditors: @bsclifton
git rebase -i
to squash commits (if needed).Test Plan: