Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add sharing current page menu items (Email, Twitter, Facebook...) #8517

Merged
merged 3 commits into from
Apr 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions app/browser/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,17 +97,19 @@ const createFileSubmenu = () => {
}
}, {
label: locale.translation('share'),
visible: false
/*
submenu: [
{label: 'Email Page Link...'},
CommonMenu.simpleShareActiveTabMenuItem('emailPageLink', 'email', 'CmdOrCtrl+Shift+I'),
CommonMenu.separatorMenuItem,
{label: 'Tweet Page...'},
{label: 'Share on Facebook...'},
{label: 'More...'}
CommonMenu.simpleShareActiveTabMenuItem('tweetPageLink', 'twitter'),
CommonMenu.simpleShareActiveTabMenuItem('facebookPageLink', 'facebook'),
CommonMenu.simpleShareActiveTabMenuItem('pinterestPageLink', 'pinterest'),
CommonMenu.simpleShareActiveTabMenuItem('googlePlusPageLink', 'googlePlus'),
CommonMenu.simpleShareActiveTabMenuItem('linkedInPageLink', 'linkedIn'),
CommonMenu.simpleShareActiveTabMenuItem('bufferPageLink', 'buffer'),
CommonMenu.simpleShareActiveTabMenuItem('redditPageLink', 'reddit')
]
*/
},
// Move inside share menu when it's enabled
CommonMenu.separatorMenuItem,
CommonMenu.printMenuItem()
]
Expand Down
21 changes: 21 additions & 0 deletions app/browser/reducers/shareReducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/* This SourceCode Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

'use strict'

const appConstants = require('../../../js/constants/appConstants')
const {makeImmutable} = require('../../common/state/immutableUtil')
const {simpleShareActiveTab} = require('../share')

const shareReducer = (state, action) => {
Copy link
Member

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?

Copy link
Member Author

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.

action = makeImmutable(action)
switch (action.get('actionType')) {
case appConstants.APP_SIMPLE_SHARE_ACTIVE_TAB_REQUESTED:
state = simpleShareActiveTab(state, action.get('windowId'), action.get('shareType'))
break
}
return state
}

module.exports = shareReducer
2 changes: 1 addition & 1 deletion app/browser/reducers/tabsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const tabsReducer = (state, action) => {
state = tabs.loadURL(state, action)
break
case appConstants.APP_LOAD_URL_IN_ACTIVE_TAB_REQUESTED:
state = tabs.loadURLInActiveTab(state, action)
state = tabs.loadURLInActiveTab(state, action.get('windowId'), action.get('url'))
break
case appConstants.APP_ON_GO_BACK:
state = tabs.goBack(state, action)
Expand Down
56 changes: 56 additions & 0 deletions app/browser/share.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const assert = require('assert')
const tabState = require('../common/state/tabState')
const {shell} = require('electron')
const tabs = require('./tabs')

const templateUrls = {
email: 'mailto:?subject={title}&body={url}',
facebook: 'https://www.facebook.com/sharer.php?u={url}',
pinterest: 'https://pinterest.com/pin/create/bookmarklet/?url={url}&description={title}',
twitter: 'https://twitter.com/intent/tweet?url={url}&text={title}&hashtags=brave',
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}',
Copy link
Member

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?

Copy link
Member Author

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.

reddit: 'https://reddit.com/submit?url={url}&title={title}'
}

const validateShareType = (shareType) =>
assert(templateUrls[shareType], 'The specified shareType is not recognized')

/**
* Performs a simple share operation for the active tab in the specified window.
*
* @param {number} windowId - The window for the active tab to use
* @param {string} shareType - The template to use, see the property key names in templateUrls above.
*/
const simpleShareActiveTab = (state, windowId, shareType) => {
const tabValue = tabState.getActiveTabValue(state, windowId)
const encodedTitle = encodeURIComponent(tabValue.get('title') || '')
const encodedUrl = encodeURIComponent(tabValue.get('url'))

validateShareType(shareType)
const templateUrl = templateUrls[shareType]

const url = templateUrl
.replace('{url}', encodedUrl)
.replace('{title}', encodedTitle)

Copy link
Member

Choose a reason for hiding this comment

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

++ on the encodeURLComponent calls 😄

if (shareType === 'email') {
shell.openExternal(url)
} else {
tabs.create({
url
})
}
return state
}

module.exports = {
simpleShareActiveTab,
templateUrls
}
15 changes: 10 additions & 5 deletions app/browser/tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -436,13 +436,18 @@ const api = {
return state
},

loadURLInActiveTab: (state, action) => {
action = makeImmutable(action)
const windowId = action.get('windowId')
loadURLInActiveTab: (state, windowId, url) => {
const tabValue = tabState.getActiveTabValue(state, windowId)
const tab = tabValue && getWebContents(tabValue.get('tabId'))
if (tabValue) {
api.loadURLInTab(state, tabValue.get('tabId'), url)
}
return state
},

loadURLInTab: (state, tabId, url) => {
const tab = getWebContents(tabId)
if (tab && !tab.isDestroyed()) {
const url = normalizeUrl(action.get('url'))
url = normalizeUrl(url)
tab.loadURL(url)
}
return state
Expand Down
10 changes: 10 additions & 0 deletions app/common/commonMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,16 @@ module.exports.printMenuItem = () => {
}
}

module.exports.simpleShareActiveTabMenuItem = (l10nId, type, accelerator) => {
return {
label: locale.translation(l10nId),
accelerator,
click: function (item, focusedWindow) {
appActions.simpleShareActiveTabRequested(getCurrentWindowId(), type)
}
}
}

module.exports.findOnPageMenuItem = () => {
return {
label: locale.translation('findOnPage'),
Expand Down
8 changes: 8 additions & 0 deletions app/extensions/brave/locales/en-US/menu.properties
Original file line number Diff line number Diff line change
Expand Up @@ -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…
Copy link
Member

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

Copy link
Member Author

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

pinterestPageLink=Share on Pinterest…
googlePlusPageLink=Share on Google+…
linkedInPageLink=Share on LinkedIn…
bufferPageLink=Share on Buffer…
redditPageLink=Share on Reddit…
findOnPage=Find on Page…
find=Find…
checkForUpdates=Check for Updates…
Expand Down
8 changes: 8 additions & 0 deletions app/locale.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ var rendererIdentifiers = function () {
'newWindow',
'reopenLastClosedTab',
'print',
'emailPageLink',
'tweetPageLink',
'facebookPageLink',
'pinterestPageLink',
'googlePlusPageLink',
'linkedInPageLink',
'bufferPageLink',
'redditPageLink',
'findOnPage',
'find',
'checkForUpdates',
Expand Down
10 changes: 10 additions & 0 deletions docs/appActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,16 @@ A request for a URL load for the active tab of the specified window



### emailActiveTabRequested(windowId)

A request for a URL email share occurred

**Parameters**

**windowId**: `number`, the window ID to use for the active tab



### maybeCreateTabRequested(createProperties)

A request for a "maybe" new tab has been made with the specified createProperties
Expand Down
13 changes: 13 additions & 0 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,19 @@ const appActions = {
})
},

/**
* A request for a URL email share occurred
* @param {number} windowId - the window ID to use for the active tab
* @param {string} shareType - The type of share to do, must be one of: "email", "facebook", "pinterest", "twitter", "googlePlus", "linkedIn", "buffer", "reddit", or "digg"
*/
simpleShareActiveTabRequested: function (windowId, shareType) {
AppDispatcher.dispatch({
actionType: appConstants.APP_SIMPLE_SHARE_ACTIVE_TAB_REQUESTED,
windowId,
shareType
})
},

/**
* A request for a "maybe" new tab has been made with the specified createProperties
* If a tab is already opened it will instead set it as active.
Expand Down
1 change: 1 addition & 0 deletions js/constants/appConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const appConstants = {
APP_MAYBE_CREATE_TAB_REQUESTED: _,
APP_LOAD_URL_REQUESTED: _,
APP_LOAD_URL_IN_ACTIVE_TAB_REQUESTED: _,
APP_SIMPLE_SHARE_ACTIVE_TAB_REQUESTED: _,
APP_NEW_WEB_CONTENTS_ADDED: _,
APP_TAB_DESTROYED: _,
APP_SET_MENUBAR_TEMPLATE: _,
Expand Down
3 changes: 2 additions & 1 deletion js/stores/appStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,8 @@ const handleAppAction = (action) => {
require('../../app/browser/reducers/passwordManagerReducer'),
require('../../app/browser/reducers/tabMessageBoxReducer'),
require('../../app/browser/reducers/dragDropReducer'),
require('../../app/browser/reducers/extensionsReducer')
require('../../app/browser/reducers/extensionsReducer'),
require('../../app/browser/reducers/shareReducer')
]
initialized = true
appState = action.appState
Expand Down
43 changes: 43 additions & 0 deletions test/unit/app/browser/reducers/shareReducerTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/* global describe, it, before, after */
const mockery = require('mockery')
const sinon = require('sinon')
const Immutable = require('immutable')
const assert = require('assert')
const fakeElectron = require('../../../lib/fakeElectron')

const appConstants = require('../../../../../js/constants/appConstants')
require('../../../braveUnit')

describe('shareReducer', function () {
let shareReducer
before(function () {
mockery.enable({
warnOnReplace: false,
warnOnUnregistered: false,
useCleanCache: true
})
mockery.registerMock('electron', fakeElectron)
this.shareStub = {
simpleShareActiveTab: sinon.stub()
}
mockery.registerMock('../share', this.shareStub)
shareReducer = require('../../../../../app/browser/reducers/shareReducer')
})

after(function () {
mockery.disable()
})

describe('APP_SIMPLE_SHARE_ACTIVE_TAB_REQUESTED', function () {
before(function () {
this.state = Immutable.Map()
this.windowId = 2
this.shareType = 'email'
this.newState = shareReducer(this.state, {actionType: appConstants.APP_SIMPLE_SHARE_ACTIVE_TAB_REQUESTED, windowId: this.windowId, shareType: this.shareType})
})
it('calls simpleShareActiveTab once with the correct args', function () {
const callCount = this.shareStub.simpleShareActiveTab.withArgs(this.state, this.windowId, this.shareType).callCount
assert.equal(callCount, 1)
})
})
})
95 changes: 95 additions & 0 deletions test/unit/app/browser/shareTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/* global describe, it, before, after, afterEach */
const mockery = require('mockery')
const sinon = require('sinon')
const Immutable = require('immutable')
const assert = require('assert')
const fakeElectron = require('../../lib/fakeElectron')

require('../../braveUnit')

describe('share API', function () {
let share
before(function () {
mockery.enable({
warnOnReplace: false,
warnOnUnregistered: false,
useCleanCache: true
})
mockery.registerMock('electron', fakeElectron)

this.mockTabs = {
create: sinon.stub()
}
mockery.registerMock('./tabs', this.mockTabs)
share = require('../../../../app/browser/share')
})

after(function () {
mockery.disable()
})

describe('simpleShareActiveTab', function () {
before(function () {
this.state = Immutable.fromJS({
tabs: [{
windowId: 2,
tabId: 2,
url: 'https://www.brave.com/2',
title: 'title 2',
active: true
}, {
windowId: 3,
tabId: 3,
url: 'https://www.brave.com/3',
title: 'title 3'
}, {
windowId: 5,
tabId: 5,
url: 'https://www.brave.com/5',
title: 'title 5',
active: true
}]
})
this.windowId = 2
this.openExternalSpy = sinon.spy(fakeElectron.shell, 'openExternal')
})
afterEach(function () {
this.openExternalSpy.reset()
this.mockTabs.create.reset()
})
describe('email', function () {
it('calls openExternal with the correct args', function () {
share.simpleShareActiveTab(this.state, 2, 'email')
const expectedUrl = 'mailto:?subject=title%202&body=https%3A%2F%2Fwww.brave.com%2F2'
const callCount = this.openExternalSpy.withArgs(expectedUrl).callCount
assert.equal(callCount, 1)
assert.equal(this.mockTabs.create.withArgs(expectedUrl).callCount, 0)
})
it('takes active tab windowId into consideration', function () {
share.simpleShareActiveTab(this.state, 5, 'email')
const expectedUrl = 'mailto:?subject=title%205&body=https%3A%2F%2Fwww.brave.com%2F5'
const callCount = this.openExternalSpy.withArgs(expectedUrl).callCount
assert.equal(callCount, 1)
assert.equal(this.mockTabs.create.withArgs(expectedUrl).callCount, 0)
})
})
describe('other', function () {
it('calls createTabRequested with correct args', function () {
Object.entries(share.templateUrls).forEach(([shareType, template]) => {
if (shareType === 'email') {
return
}
share.simpleShareActiveTab(this.state, 5, shareType)
const expectedUrl = template
.replace('{url}', 'https%3A%2F%2Fwww.brave.com%2F5')
.replace('{title}', 'title%205')
assert.equal(this.openExternalSpy.withArgs({url: expectedUrl}).callCount, 0)
assert.equal(this.mockTabs.create.callCount, 1)
assert.equal(this.mockTabs.create.args[0][0].url, expectedUrl)
this.openExternalSpy.reset()
this.mockTabs.create.reset()
})
})
})
})
})
Loading