From cd35e14096f07633af9ae863df1f9b329df3c1bd Mon Sep 17 00:00:00 2001 From: NejcZdovc Date: Thu, 18 May 2017 14:27:49 +0200 Subject: [PATCH] Refactors addEditBookmarHanger and addEditBookmark to redux Resolves #8927 Auditors: @bsclifton Test Plan: - try to bookmark a page --- app/common/lib/bookmarkUtil.js | 38 +++++ .../components/bookmarks/addEditBookmark.js | 38 ----- .../bookmarks/addEditBookmarkHanger.js | 151 +++++++++--------- app/renderer/components/main/main.js | 12 +- .../components/navigation/navigationBar.js | 17 +- app/renderer/components/navigation/urlBar.js | 2 + test/unit/app/common/lib/bookmarkUtilTest.js | 151 ++++++++++++++++++ 7 files changed, 274 insertions(+), 135 deletions(-) create mode 100644 app/common/lib/bookmarkUtil.js delete mode 100644 app/renderer/components/bookmarks/addEditBookmark.js create mode 100644 test/unit/app/common/lib/bookmarkUtilTest.js diff --git a/app/common/lib/bookmarkUtil.js b/app/common/lib/bookmarkUtil.js new file mode 100644 index 00000000000..58550f3036a --- /dev/null +++ b/app/common/lib/bookmarkUtil.js @@ -0,0 +1,38 @@ +/* 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/. */ + +function bookmarkHangerHeading (detail, isFolder, shouldShowLocation) { + if (isFolder) { + return shouldShowLocation + ? 'bookmarkFolderEditing' + : 'bookmarkFolderAdding' + } + return shouldShowLocation + ? (!detail || !detail.has('location')) + ? 'bookmarkCreateNew' + : 'bookmarkEdit' + : 'bookmarkAdded' +} + +function displayBookmarkName (detail) { + const customTitle = detail.get('customTitle') + if (customTitle !== undefined && customTitle !== '') { + return customTitle || '' + } + return detail.get('title') || '' +} + +function isBookmarkNameValid (detail, isFolder) { + const title = detail.get('title') || detail.get('customTitle') + const location = detail.get('location') + return isFolder + ? (title != null && title.trim().length > 0) + : (location != null && location.trim().length > 0) +} + +module.exports = { + bookmarkHangerHeading, + displayBookmarkName, + isBookmarkNameValid +} diff --git a/app/renderer/components/bookmarks/addEditBookmark.js b/app/renderer/components/bookmarks/addEditBookmark.js deleted file mode 100644 index 575bee7c917..00000000000 --- a/app/renderer/components/bookmarks/addEditBookmark.js +++ /dev/null @@ -1,38 +0,0 @@ -/* 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 React = require('react') - -// Components -const ImmutableComponent = require('../immutableComponent') -const AddEditBookmarkHanger = require('./addEditBookmarkHanger') - -// Actions -const windowActions = require('../../../../js/actions/windowActions') - -class AddEditBookmark extends ImmutableComponent { - constructor () { - super() - this.onClose = this.onClose.bind(this) - } - onClose () { - windowActions.setBookmarkDetail() - } - componentDidMount () { - this.refs.bookmarkHanger.setDefaultFocus() - } - render () { - return - } -} - -module.exports = AddEditBookmark diff --git a/app/renderer/components/bookmarks/addEditBookmarkHanger.js b/app/renderer/components/bookmarks/addEditBookmarkHanger.js index d1f3d59269f..ef18ba70b3f 100644 --- a/app/renderer/components/bookmarks/addEditBookmarkHanger.js +++ b/app/renderer/components/bookmarks/addEditBookmarkHanger.js @@ -5,7 +5,7 @@ const React = require('react') // Components -const ImmutableComponent = require('../immutableComponent') +const ReduxComponent = require('../reduxComponent') const Dialog = require('../common/dialog') const BrowserButton = require('../common/browserButton') @@ -22,7 +22,8 @@ const settings = require('../../../../js/constants/settings') const cx = require('../../../../js/lib/classSet') const siteUtil = require('../../../../js/state/siteUtil') const UrlUtil = require('../../../../js/lib/urlutil') -const getSetting = require('../../../../js/settings').getSetting +const {getSetting} = require('../../../../js/settings') +const {bookmarkHangerHeading, displayBookmarkName, isBookmarkNameValid} = require('../../../common/lib/bookmarkUtil') const {StyleSheet, css} = require('aphrodite/no-important') const globalStyles = require('../styles/global') @@ -38,9 +39,9 @@ const { commonFormStyles } = require('../common/commonForm') -class AddEditBookmarkHanger extends ImmutableComponent { - constructor () { - super() +class AddEditBookmarkHanger extends React.Component { + constructor (props) { + super(props) this.onNameChange = this.onNameChange.bind(this) this.onLocationChange = this.onLocationChange.bind(this) this.onParentFolderChange = this.onParentFolderChange.bind(this) @@ -51,60 +52,21 @@ class AddEditBookmarkHanger extends ImmutableComponent { this.onViewBookmarks = this.onViewBookmarks.bind(this) this.onRemoveBookmark = this.onRemoveBookmark.bind(this) } - get bookmarkNameValid () { - const title = this.props.currentDetail.get('title') || this.props.currentDetail.get('customTitle') - const location = this.props.currentDetail.get('location') - return this.isFolder - ? (typeof title === 'string' && title.trim().length > 0) - : (typeof location === 'string' && location.trim().length > 0) - } - get displayBookmarkName () { - const customTitle = this.props.currentDetail.get('customTitle') - if (customTitle !== undefined && customTitle !== '') { - return customTitle || '' - } - return this.props.currentDetail.get('title') || '' - } - get heading () { - if (this.isFolder) { - return this.props.shouldShowLocation - ? 'bookmarkFolderEditing' - : 'bookmarkFolderAdding' - } - return this.props.shouldShowLocation - ? (!this.props.originalDetail || !this.props.originalDetail.has('location')) - ? 'bookmarkCreateNew' - : 'bookmarkEdit' - : 'bookmarkAdded' - } - get isFolder () { - // Fake a folderId property so that the bookmark is considered a bookmark folder. - // This is ImmutableJS so it doesn't actually set a value, it just returns a new one. - return siteUtil.isFolder(this.props.currentDetail.set('folderId', 0)) - } + setDefaultFocus () { this.bookmarkName.select() this.bookmarkName.focus() } - updateFolders (props) { - this.folders = siteUtil.getFolders(this.props.sites, props.currentDetail.get('folderId')) - } - componentWillMount () { - this.updateFolders(this.props) - } - componentWillUpdate (nextProps) { - if (this.props.sites !== nextProps.sites) { - this.updateFolders(nextProps) - } - } + componentDidMount () { // Automatically save if this is triggered by the url star if (!this.props.isModal && !this.props.shouldShowLocation) { this.onSave(false) } - this.bookmarkName.value = this.displayBookmarkName + this.bookmarkName.value = displayBookmarkName(this.props.currentDetail) this.setDefaultFocus() } + onKeyDown (e) { switch (e.keyCode) { case KeyCodes.ENTER: @@ -115,12 +77,15 @@ class AddEditBookmarkHanger extends ImmutableComponent { break } } + onClose () { windowActions.setBookmarkDetail() } + onClick (e) { e.stopPropagation() } + onNameChange (e) { let currentDetail = this.props.currentDetail let name = e.target.value @@ -135,7 +100,7 @@ class AddEditBookmarkHanger extends ImmutableComponent { } else { // '' is reserved for the default customTitle value of synced bookmarks, // so replace '' with 0 if the user is deleting the customTitle. - // Note that non-string bookmark titles fail bookmarkNameValid so they + // Note that non-string bookmark titles fail isBookmarkNameValid so they // are not saved. currentDetail = currentDetail.set('customTitle', name || 0) } @@ -144,6 +109,7 @@ class AddEditBookmarkHanger extends ImmutableComponent { } windowActions.setBookmarkDetail(currentDetail, this.props.originalDetail, this.props.destinationDetail, this.props.shouldShowLocation, !this.props.isModal) } + onLocationChange (e) { let location = e.target.value if (typeof location === 'string') { @@ -155,62 +121,95 @@ class AddEditBookmarkHanger extends ImmutableComponent { const currentDetail = this.props.currentDetail.set('location', location) windowActions.setBookmarkDetail(currentDetail, this.props.originalDetail, this.props.destinationDetail, this.props.shouldShowLocation, !this.props.isModal) } + onParentFolderChange (e) { const currentDetail = this.props.currentDetail.set('parentFolderId', Number(e.target.value)) windowActions.setBookmarkDetail(currentDetail, this.props.originalDetail, this.props.destinationDetail, undefined, !this.props.isModal) } + showToolbarOnFirstBookmark () { - const hasBookmarks = this.props.sites.find( - (site) => siteUtil.isBookmark(site) || siteUtil.isFolder(site) - ) - if (!hasBookmarks && !getSetting(settings.SHOW_BOOKMARKS_TOOLBAR)) { + if (!this.props.hasBookmarks && !getSetting(settings.SHOW_BOOKMARKS_TOOLBAR)) { appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, true) } } + onSave (closeDialog = true) { // First check if the title of the currentDetail is set - if (!this.bookmarkNameValid) { + if (!this.props.isBookmarkNameValid) { return false } this.showToolbarOnFirstBookmark() - const tag = this.isFolder ? siteTags.BOOKMARK_FOLDER : siteTags.BOOKMARK + const tag = this.props.isFolder ? siteTags.BOOKMARK_FOLDER : siteTags.BOOKMARK appActions.addSite(this.props.currentDetail, tag, this.props.originalDetail, this.props.destinationDetail) + if (closeDialog) { this.onClose() } } + onRemoveBookmark () { - const tag = this.isFolder ? siteTags.BOOKMARK_FOLDER : siteTags.BOOKMARK + const tag = this.props.isFolder ? siteTags.BOOKMARK_FOLDER : siteTags.BOOKMARK appActions.removeSite(this.props.currentDetail, tag) this.onClose() } + onViewBookmarks () { this.onClose() appActions.createTabRequested({url: 'about:bookmarks'}) } - render () { - const arrowUp = this.props.isModal - ? null - : { - className: cx({ - [css(styles.bookmarkHanger__arrowUp)]: true, - [css(styles.bookmarkHanger__withHomeButton)]: this.props.withHomeButton, - withStopButton: this.props.withStopButton, - withoutButtons: this.props.withoutButtons - }) - } + mergeProps (state, dispatchProps, ownProps) { + const currentWindow = state.get('currentWindow') + const bookmarkDetail = currentWindow.get('bookmarkDetail') + const currentDetail = bookmarkDetail.get('currentDetail') + const originalDetail = bookmarkDetail.get('originalDetail') + + const props = {} + // used in renderer + props.isModal = ownProps.isModal + props.withHomeButton = getSetting(settings.SHOW_HOME_BUTTON) + // Fake a folderId property so that the bookmark is considered a bookmark folder. + // This is ImmutableJS so it doesn't actually set a value, it just returns a new one. + props.isFolder = siteUtil.isFolder(currentDetail.set('folderId', 0)) + props.shouldShowLocation = bookmarkDetail.get('shouldShowLocation') + props.heading = bookmarkHangerHeading(originalDetail, props.isFolder, props.shouldShowLocation) + props.location = currentDetail.get('location') + props.parentFolderId = currentDetail.get('parentFolderId') + props.hasOriginalDetail = !!originalDetail + props.displayBookmarkName = displayBookmarkName(currentDetail) + props.isBookmarkNameValid = isBookmarkNameValid(currentDetail, props.isFolder) + props.folders = siteUtil.getFolders(state.get('sites'), currentDetail.get('folderId')) // TODO (nejc) improve, primitives only + + // used in functions + props.currentDetail = currentDetail // TODO (nejc) improve, primitives only + props.originalDetail = originalDetail // TODO (nejc) improve, primitives only + props.destinationDetail = bookmarkDetail.get('destinationDetail') // TODO (nejc) improve, primitives only + props.hasBookmarks = state.get('sites').find( + (site) => siteUtil.isBookmark(site) || siteUtil.isFolder(site) + ) + + return props + } + + render () { return -
+ { + !this.props.isModal + ?
+ : null + }
+ })} data-l10n-id={this.props.heading} />
@@ -235,7 +234,7 @@ class AddEditBookmarkHanger extends ImmutableComponent {
{ - !this.isFolder && this.props.shouldShowLocation + !this.props.isFolder && this.props.shouldShowLocation ?
@@ -263,11 +262,11 @@ class AddEditBookmarkHanger extends ImmutableComponent { data-l10n-id='parentFolderField' htmlFor='bookmarkParentFolder' /> ) + this.props.folders.map((folder) => ) }
@@ -275,7 +274,7 @@ class AddEditBookmarkHanger extends ImmutableComponent { { - this.props.originalDetail + this.props.hasOriginalDetail ? @@ -372,4 +371,4 @@ const styles = StyleSheet.create({ } }) -module.exports = AddEditBookmarkHanger +module.exports = ReduxComponent.connect(AddEditBookmarkHanger) diff --git a/app/renderer/components/main/main.js b/app/renderer/components/main/main.js index c3f254e7596..d165cd13a5e 100644 --- a/app/renderer/components/main/main.js +++ b/app/renderer/components/main/main.js @@ -31,7 +31,7 @@ const ImportBrowserDataPanel = require('./importBrowserDataPanel') const WidevinePanel = require('./widevinePanel') const AutofillAddressPanel = require('../autofill/autofillAddressPanel') const AutofillCreditCardPanel = require('../autofill/autofillCreditCardPanel') -const AddEditBookmark = require('../bookmarks/addEditBookmark') +const AddEditBookmarkHanger = require('../bookmarks/addEditBookmarkHanger') const LoginRequired = require('./loginRequired') const ReleaseNotes = require('./releaseNotes') const BookmarksToolbar = require('../bookmarks/bookmarksToolbar') @@ -794,17 +794,13 @@ class Main extends ImmutableComponent { } { this.props.windowState.get('bookmarkDetail') && !this.props.windowState.getIn(['bookmarkDetail', 'isBookmarkHanger']) - ? + ? : null } { noScriptIsVisible - ? : null } diff --git a/app/renderer/components/navigation/navigationBar.js b/app/renderer/components/navigation/navigationBar.js index 14f35d673c3..1164891b29a 100644 --- a/app/renderer/components/navigation/navigationBar.js +++ b/app/renderer/components/navigation/navigationBar.js @@ -25,7 +25,6 @@ const settings = require('../../../../js/constants/settings') // State const tabState = require('../../../common/state/tabState') const frameStateUtil = require('../../../../js/state/frameStateUtil') -const menuBarState = require('../../../common/state/menuBarState') // Store const windowStore = require('../../../../js/stores/windowStore') @@ -151,7 +150,7 @@ class NavigationBar extends React.Component { const props = {} props.navbar = navbar - props.sites = state.get('sites') + props.sites = state.get('sites') // TODO(nejc) remove, primitives only props.activeFrameKey = activeFrameKey props.location = location props.title = title @@ -159,10 +158,9 @@ class NavigationBar extends React.Component { props.partitionNumber = activeFrame.get('partitionNumber') || 0 props.isSecure = activeFrame.getIn(['security', 'isSecure']) props.loading = loading - props.bookmarkDetail = bookmarkDetail + props.showBookmarkHanger = bookmarkDetail && bookmarkDetail.get('isBookmarkHanger') props.mouseInTitlebar = mouseInTitlebar props.settings = state.get('settings') - props.menubarVisible = menuBarState.isMenuBarVisible(currentWindow) props.siteSettings = state.get('siteSettings') props.synopsis = state.getIn(['publisherInfo', 'synopsis']) || new Immutable.Map() props.activeTabShowingMessageBox = activeTabShowingMessageBox @@ -188,14 +186,8 @@ class NavigationBar extends React.Component { [css(styles.navigator_wide)]: this.props.isWideURLbarEnabled })}> { - this.props.bookmarkDetail && this.props.bookmarkDetail.get('isBookmarkHanger') - ? + this.props.showBookmarkHanger + ? : null } { @@ -246,7 +238,6 @@ class NavigationBar extends React.Component { { isSourceAboutUrl(this.props.location) diff --git a/app/renderer/components/navigation/urlBar.js b/app/renderer/components/navigation/urlBar.js index 4647b61ab55..0576f1a1a8a 100644 --- a/app/renderer/components/navigation/urlBar.js +++ b/app/renderer/components/navigation/urlBar.js @@ -26,6 +26,7 @@ const frameStateUtil = require('../../../../js/state/frameStateUtil') const siteSettings = require('../../../../js/state/siteSettings') const tabState = require('../../../common/state/tabState') const siteSettingsState = require('../../../common/state/siteSettingsState') +const menuBarState = require('../../../common/state/menuBarState') // Utils const urlParse = require('../../../common/urlParse') @@ -533,6 +534,7 @@ class UrlBar extends React.Component { props.autocompleteEnabled = urlbar.getIn(['suggestions', 'autocompleteEnabled']) props.searchURL = searchURL props.searchShortcut = searchShortcut + props.menubarVisible = menuBarState.isMenuBarVisible(currentWindow) return props } diff --git a/test/unit/app/common/lib/bookmarkUtilTest.js b/test/unit/app/common/lib/bookmarkUtilTest.js new file mode 100644 index 00000000000..9a53ed006df --- /dev/null +++ b/test/unit/app/common/lib/bookmarkUtilTest.js @@ -0,0 +1,151 @@ +/* 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/. */ + +/* global describe, it */ +const assert = require('assert') +const bookmarkUtil = require('../../../../../app/common/lib/bookmarkUtil') +const {makeImmutable} = require('../../../../../app/common/state/immutableUtil') + +require('../../../braveUnit') + +describe('bookmarkUtil test', function () { + describe('bookmarkHangerHeading', function () { + const detail = makeImmutable({ + location: 'https://ww.brave.com' + }) + + it('returns default if isFolder and shouldShowLocation are not provided', function () { + assert.equal(bookmarkUtil.bookmarkHangerHeading(detail), 'bookmarkAdded') + }) + + describe('is folder', function () { + it('returns editing mode when shouldShowLocation is not provided', function () { + assert.equal(bookmarkUtil.bookmarkHangerHeading(detail, true, true), 'bookmarkFolderEditing') + }) + + it('returns editing mode when shouldShowLocation is true', function () { + assert.equal(bookmarkUtil.bookmarkHangerHeading(detail, true, true), 'bookmarkFolderEditing') + }) + + it('returns add mode when shouldShowLocation is false', function () { + assert.equal(bookmarkUtil.bookmarkHangerHeading(detail, true, false), 'bookmarkFolderAdding') + }) + }) + + describe('is bookmark', function () { + it('returns add mode when shouldShowLocation is false', function () { + assert.equal(bookmarkUtil.bookmarkHangerHeading(detail, false, false), 'bookmarkAdded') + }) + + it('returns create mode when details is empty', function () { + assert.equal(bookmarkUtil.bookmarkHangerHeading(makeImmutable({}), false, true), 'bookmarkCreateNew') + }) + + it('returns edit mode when details is provided', function () { + assert.equal(bookmarkUtil.bookmarkHangerHeading(detail, false, true), 'bookmarkEdit') + }) + }) + }) + + describe('displayBookmarkName', function () { + it('custom title', function () { + it('is not provided', function () { + assert.equal(bookmarkUtil.displayBookmarkName(makeImmutable({ + title: 'brave' + })), 'brave') + }) + + it('is null', function () { + assert.equal(bookmarkUtil.displayBookmarkName(makeImmutable({ + customTitle: null + })), '') + }) + + it('is provided', function () { + assert.equal(bookmarkUtil.displayBookmarkName(makeImmutable({ + customTitle: 'custom brave' + })), 'custom brave') + }) + }) + + it('regular title', function () { + it('is not provided', function () { + assert.equal(bookmarkUtil.displayBookmarkName(makeImmutable({})), '') + }) + + it('is null', function () { + assert.equal(bookmarkUtil.displayBookmarkName(makeImmutable({ + title: null + })), '') + }) + + it('is provided', function () { + assert.equal(bookmarkUtil.displayBookmarkName(makeImmutable({ + title: 'brave' + })), 'brave') + }) + }) + }) + + describe('isBookmarkNameValid', function () { + describe('for folder', function () { + it('title and custom title is not provided', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({ + location: 'https://ww.brave.com' + }), true), false) + }) + + it('title and custom title are null', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({ + title: null, + customTitle: null + }), true), false) + }) + + it('title is empty string', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({ + title: '' + }), true), false) + }) + + it('title is null, but customTitle is ok', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({ + title: null, + customTitle: 'custom brave' + }), true), true) + }) + + it('title and customTitle are ok', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({ + title: 'brave', + customTitle: 'custom brave' + }), true), true) + }) + }) + + describe('for bookmark', function () { + it('location is not provided', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({}), true), false) + }) + + it('location is null', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({ + location: null + }), false), false) + }) + + it('location is empty string', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({ + location: '' + }), false), false) + }) + + it('location is provided', function () { + assert.equal(bookmarkUtil.isBookmarkNameValid(makeImmutable({ + location: 'https://ww.brave.com' + }), false), true) + }) + }) + }) +})