Merge pull request #4726 from brave/bookmark-popup

Improve UI/UX of bookmark popup

app/extensions/brave/locales/en-US/app.properties

@@ -129,9 +129,9 @@ allowFlashPlayer=Allow {{origin}} to run Flash Player?
 ledgerBackupText=Your ledger keys are {{paymentId}} and {{passphrase}}
 error=Error
 caseSensitivity=Match case
-nameField=Title:
-locationField=Location:
-parentFolderField=Folder:
+nameField=Title
+locationField=Location
+parentFolderField=Folder
 save=Save
 rememberDecision=Remember this decision
 bookmarksToolbar=Bookmarks Toolbar

app/extensions/brave/locales/en-US/menu.properties

@@ -3,6 +3,8 @@ closeTab=Close
 closeOtherTabs=Close other Tabs
 bookmarkPage=Bookmark Page
 bookmarkLink=Bookmark This Link
+bookmarkAdded=Bookmark Added
+bookmarkEdit=Edit Bookmark
 openFile=Open File…
 openLocation=Open Location…
 openSearch=Search for "{{selectedVariable}}"
@@ -19,7 +21,8 @@ paste=Paste
 pasteAndGo=Paste and Go
 pasteAndSearch=Paste and Search
 pasteWithoutFormatting=Paste without formatting
-delete=Delete
+done=Done
+remove=Remove
 selectAll=Select All
 findNext=Find Next
 findPrevious=Find Previous
@@ -50,6 +53,7 @@ clearSiteData=Clear All Cookies and Site Data…
 recentlyClosed=Recently Closed
 recentlyVisited=Recently Visited
 bookmarks=Bookmarks
+viewBookmarks=View all bookmarks
 addToFavoritesBar=Add to Favorites Bar
 window=Window
 minimize=Minimize

docs/state.md

@@ -372,6 +372,7 @@ WindowStore
  bookmarkDetail: {
  currentDetail: Object, // Detail of the current bookmark which is in add/edit mode
  originalDetails: Object // Detail of the original bookmark to edit
+ shouldShowLocation: Boolean // Whether or not to show the URL input
  },
  braveryPanelDetail: {
  advancedControls: boolean, // If specified, indicates if advanced controls should be shown

docs/windowActions.md

@@ -467,7 +467,7 @@ Dispatches a message to set the find-in-page details.
 
 
 
-### setBookmarkDetail(currentDetail, originalDetail, destinationDetail) 
+### setBookmarkDetail(currentDetail, originalDetail, destinationDetail, shouldShowLocation) 
 
 Dispatches a message to set add/edit bookmark details
 If set, also indicates that add/edit is shown
@@ -480,6 +480,8 @@ If set, also indicates that add/edit is shown
 
 **destinationDetail**: `Object`, Will move the added bookmark to the specified position
 
+**shouldShowLocation**: `Object`, Whether or not to show the URL input
+
 
 
 ### setContextMenuDetail(detail) 

js/actions/windowActions.js

@@ -643,13 +643,15 @@ const windowActions = {
  * @param {Object} currentDetail - Properties of the bookmark to change to
  * @param {Object} originalDetail - Properties of the bookmark to edit
  * @param {Object} destinationDetail - Will move the added bookmark to the specified position
+ * @param {Object} shouldShowLocation - Whether or not to show the URL input
  */
- setBookmarkDetail: function (currentDetail, originalDetail, destinationDetail) {
+ setBookmarkDetail: function (currentDetail, originalDetail, destinationDetail, shouldShowLocation) {
  dispatch({
  actionType: WindowConstants.WINDOW_SET_BOOKMARK_DETAIL,
  currentDetail,
  originalDetail,
- destinationDetail
+ destinationDetail,
+ shouldShowLocation
  })
  },
 

js/components/addEditBookmark.js

@@ -4,8 +4,8 @@
 
 const React = require('react')
 const ImmutableComponent = require('./immutableComponent')
-const Dialog = require('./dialog')
 const Button = require('./button')
+const cx = require('../lib/classSet')
 const windowActions = require('../actions/windowActions')
 const appActions = require('../actions/appActions')
 const KeyCodes = require('../../app/common/constants/keyCodes')
@@ -24,6 +24,7 @@ class AddEditBookmark extends ImmutableComponent {
  this.onClose = this.onClose.bind(this)
  this.onClick = this.onClick.bind(this)
  this.onSave = this.onSave.bind(this)
+ this.onViewBookmarks = this.onViewBookmarks.bind(this)
  this.onRemoveBookmark = this.onRemoveBookmark.bind(this)
  }
 
@@ -51,6 +52,10 @@ class AddEditBookmark extends ImmutableComponent {
  }
  }
  componentDidMount () {
+ // Automatically save if this is triggered by the url star
+ if (!this.props.shouldShowLocation) {
+ this.onSave(false)
+ }
  this.bookmarkName.select()
  this.bookmarkName.focus()
  }
@@ -96,7 +101,7 @@ class AddEditBookmark extends ImmutableComponent {
  appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, true)
  }
  }
- onSave () {
+ onSave (closeDialog = true) {
  // First check if the title of the currentDetail is set
  if (!this.bookmarkNameValid) {
  return false
@@ -105,56 +110,78 @@ class AddEditBookmark extends ImmutableComponent {
  this.showToolbarOnFirstBookmark()
  const tag = this.isFolder ? siteTags.BOOKMARK_FOLDER : siteTags.BOOKMARK
  appActions.addSite(this.props.currentDetail, tag, this.props.originalDetail, this.props.destinationDetail)
- this.onClose()
+ if (closeDialog) {
+ this.onClose()
+ }
  }
  onRemoveBookmark () {
  const tag = this.isFolder ? siteTags.BOOKMARK_FOLDER : siteTags.BOOKMARK
  appActions.removeSite(this.props.currentDetail, tag)
  this.onClose()
  }
+ onViewBookmarks () {
+ this.onClose()
+ windowActions.newFrame({location: 'about:bookmarks'}, true)
+ }
  get displayBookmarkName () {
  if (this.props.currentDetail.get('customTitle') !== undefined) {
  return this.props.currentDetail.get('customTitle')
  }
  return this.props.currentDetail.get('title') || ''
  }
  render () {
- return <Dialog onHide={this.onClose} isClickDismiss>
- <div className='genericForm' onClick={this.onClick}>
- <div className='genericFormTable'>
- <div id='bookmarkName' className='formRow'>
- <label data-l10n-id='nameField' htmlFor='bookmarkName' />
- <input spellCheck='false' onKeyDown={this.onKeyDown} onChange={this.onNameChange} value={this.displayBookmarkName} ref={(bookmarkName) => { this.bookmarkName = bookmarkName }} />
- </div>
+ return <div className='bookmarkDialog'>
+ <div className='bookmarkForm' onClick={this.onClick}>
+ <div className={cx({
+ arrowUp: true,
+ withStopButton: this.props.withStopButton,
+ withHomeButton: this.props.withHomeButton,
+ withoutButtons: this.props.withoutButtons
+ })} />
+ <div className='bookmarkFormInner'>
  {
- !this.isFolder
- ? <div id='bookmarkLocation' className='formRow'>
- <label data-l10n-id='locationField' htmlFor='bookmarkLocation' />
- <input spellCheck='false' onKeyDown={this.onKeyDown} onChange={this.onLocationChange} value={this.props.currentDetail.get('location')} />
- </div>
- : null
+ !this.isFolder && this.props.shouldShowLocation
+ ? <h2 data-l10n-id='bookmarkEdit' />
+ : <h2 data-l10n-id='bookmarkAdded' />
  }
- <div id='bookmarkParentFolder' className='formRow'>
- <label data-l10n-id='parentFolderField' htmlFor='bookmarkParentFolderk' />
- <select value={this.props.currentDetail.get('parentFolderId')}
- onChange={this.onParentFolderChange} >
- <option value='0' data-l10n-id='bookmarksToolbar' />
- {
- this.folders.map((folder) => <option value={folder.folderId}>{folder.label}</option>)
- }
- </select>
- </div>
- <div className='formRow'>
+ <div className='bookmarkFormTable'>
+ <div id='bookmarkName' className='bookmarkFormRow'>
+ <label data-l10n-id='nameField' htmlFor='bookmarkName' />
+ <input spellCheck='false' onKeyDown={this.onKeyDown} onChange={this.onNameChange} value={this.displayBookmarkName} ref={(bookmarkName) => { this.bookmarkName = bookmarkName }} />
+ </div>
  {
- this.props.originalDetail
- ? <a data-l10n-id='delete' className='removeBookmarkLink link' onClick={this.onRemoveBookmark} />
+ !this.isFolder && this.props.shouldShowLocation
+ ? <div id='bookmarkLocation' className='bookmarkFormRow'>
+ <label data-l10n-id='locationField' htmlFor='bookmarkLocation' />
+ <input spellCheck='false' onKeyDown={this.onKeyDown} onChange={this.onLocationChange} value={this.props.currentDetail.get('location')} />
+ </div>
  : null
  }
- <Button l10nId='save' disabled={!this.bookmarkNameValid} className='primaryButton' onClick={this.onSave} />
+ <div id='bookmarkParentFolder' className='bookmarkFormRow'>
+ <label data-l10n-id='parentFolderField' htmlFor='bookmarkParentFolderk' />
+ <select value={this.props.currentDetail.get('parentFolderId')}
+ onChange={this.onParentFolderChange} >
+ <option value='0' data-l10n-id='bookmarksToolbar' />
+ {
+ this.folders.map((folder) => <option value={folder.folderId}>{folder.label}</option>)
+ }
+ </select>
+ </div>
+ <div className='bookmarkButtons'>
+ {
+ this.props.originalDetail
+ ? <Button l10nId='remove' className='primaryButton whiteButton inlineButton' onClick={this.onRemoveBookmark} />
+ : null
+ }
+ <Button l10nId='done' disabled={!this.bookmarkNameValid} className='primaryButton' onClick={this.onSave} />
+ </div>
  </div>
  </div>
+ <div className='bookmarkFormFooter'>
+ <Button l10nId='viewBookmarks' onClick={this.onViewBookmarks} />
+ </div>
  </div>
- </Dialog>
+ </div>
  }
 }
 

js/components/main.js

@@ -32,7 +32,6 @@ const ClearBrowsingDataPanel = require('./clearBrowsingDataPanel')
 const ImportBrowserDataPanel = require('../../app/renderer/components/importBrowserDataPanel')
 const AutofillAddressPanel = require('./autofillAddressPanel')
 const AutofillCreditCardPanel = require('./autofillCreditCardPanel')
-const AddEditBookmark = require('./addEditBookmark')
 const LoginRequired = require('./loginRequired')
 const ReleaseNotes = require('./releaseNotes')
 const BookmarksToolbar = require('../../app/renderer/components/bookmarksToolbar')
@@ -667,7 +666,8 @@ class Main extends ImmutableComponent {
  (node.classList.contains('popupWindow') ||
  node.classList.contains('contextMenu') ||
  node.classList.contains('extensionButton') ||
- node.classList.contains('menubarItem'))) {
+ node.classList.contains('menubarItem') ||
+ node.classList.contains('bookmarkForm'))) {
  // Middle click (on context menu) needs to fire the click event.
  // We need to prevent the default "Auto-Scrolling" behavior.
  if (node.classList.contains('contextMenu') && e.button === 1) {
@@ -919,6 +919,7 @@ class Main extends ImmutableComponent {
  startLoadTime={activeFrame && activeFrame.get('startLoadTime') || undefined}
  endLoadTime={activeFrame && activeFrame.get('endLoadTime') || undefined}
  loading={activeFrame && activeFrame.get('loading')}
+ bookmarkDetail={this.props.windowState.get('bookmarkDetail')}
  mouseInTitlebar={this.props.windowState.getIn(['ui', 'mouseInTitlebar'])}
  searchDetail={this.props.windowState.get('searchDetail')}
  enableNoScript={this.enableNoScript(activeSiteSettings)}
@@ -1003,14 +1004,6 @@ class Main extends ImmutableComponent {
  ? <LoginRequired loginRequiredDetail={loginRequiredDetail} tabId={activeFrame.get('tabId')} />
  : null
  }
- {
- this.props.windowState.get('bookmarkDetail')
- ? <AddEditBookmark sites={this.props.appState.get('sites')}
- currentDetail={this.props.windowState.getIn(['bookmarkDetail', 'currentDetail'])}
- originalDetail={this.props.windowState.getIn(['bookmarkDetail', 'originalDetail'])}
- destinationDetail={this.props.windowState.getIn(['bookmarkDetail', 'destinationDetail'])} />
- : null
- }
  {
  noScriptIsVisible
  ? <NoScriptInfo frameProps={activeFrame}

js/components/navigationBar.js

@@ -15,6 +15,7 @@ const messages = require('../constants/messages')
 const settings = require('../constants/settings')
 const ipc = global.require('electron').ipcRenderer
 const {isSourceAboutUrl} = require('../lib/appUrlUtil')
+const AddEditBookmark = require('./addEditBookmark')
 const siteUtil = require('../state/siteUtil')
 const eventUtil = require('../lib/eventUtil')
 const getSetting = require('../settings').getSetting
@@ -38,9 +39,10 @@ class NavigationBar extends ImmutableComponent {
  }
 
  onToggleBookmark () {
+ const editing = this.bookmarked
  // trigger the AddEditBookmark modal; saving/deleting takes place there
  const siteDetail = siteUtil.getDetailFromFrame(this.activeFrame, siteTags.BOOKMARK)
- windowActions.setBookmarkDetail(siteDetail, siteDetail)
+ windowActions.setBookmarkDetail(siteDetail, siteDetail, null, editing)
  }
 
  onReload (e) {
@@ -73,6 +75,7 @@ class NavigationBar extends ImmutableComponent {
 
  get titleMode () {
  return this.props.mouseInTitlebar === false &&
+ !this.props.bookmarkDetail &&
  this.props.title &&
  !['about:blank', 'about:newtab'].includes(this.props.location) &&
  !this.loading &&
@@ -112,10 +115,23 @@ class NavigationBar extends ImmutableComponent {
  className={cx({
  titleMode: this.titleMode
  })}>
+ {
+ this.props.bookmarkDetail
+ ? <AddEditBookmark sites={this.props.sites}
+ currentDetail={this.props.bookmarkDetail.get('currentDetail')}
+ originalDetail={this.props.bookmarkDetail.get('originalDetail')}
+ destinationDetail={this.props.bookmarkDetail.get('destinationDetail')}
+ shouldShowLocation={this.props.bookmarkDetail.get('shouldShowLocation')}
+ withStopButton={!isSourceAboutUrl(this.props.location)}
+ withHomeButton={getSetting(settings.SHOW_HOME_BUTTON)}
+ withoutButtons={isSourceAboutUrl(this.props.location) && !getSetting(settings.SHOW_HOME_BUTTON)}
+ />
+ : null
+ }
  <div className='startButtons'>
  {
  isSourceAboutUrl(this.props.location) || this.titleMode
- ? <span className='browserButton' />
+ ? null
  : this.loading
  ? <Button iconClass='fa-times'
  l10nId='stopButton'
@@ -134,6 +150,17 @@ class NavigationBar extends ImmutableComponent {
  onClick={this.onHome} />
  : null
  }
+ <Button iconClass={this.bookmarked ? 'fa-star' : 'fa-star-o'}
+ className={cx({
+ navbutton: true,
+ bookmarkButton: true,
+ removeBookmarkButton: this.bookmarked,
+ withStopButton: !isSourceAboutUrl(this.props.location),
+ withHomeButton: getSetting(settings.SHOW_HOME_BUTTON),
+ withoutButtons: isSourceAboutUrl(this.props.location) && !getSetting(settings.SHOW_HOME_BUTTON)
+ })}
+ l10nId={this.bookmarked ? 'removeBookmarkButton' : 'addBookmarkButton'}
+ onClick={this.onToggleBookmark} />
  </div>
  <UrlBar ref='urlBar'
  sites={this.props.sites}
@@ -168,14 +195,6 @@ class NavigationBar extends ImmutableComponent {
  })}
  onClick={this.onNoScript} />
  }
- <Button iconClass={this.bookmarked ? 'fa-star' : 'fa-star-o'}
- className={cx({
- navbutton: true,
- bookmarkButton: true,
- removeBookmarkButton: this.bookmarked
- })}
- l10nId={this.bookmarked ? 'removeBookmarkButton' : 'addBookmarkButton'}
- onClick={this.onToggleBookmark} />
  </div>
  }
  </div>

js/constants/windowConstants.js

@@ -46,6 +46,7 @@ const windowConstants = {
  WINDOW_SET_BOOKMARK_DETAIL: _, // If set, also indicates that add/edit is shown
  WINDOW_SET_CONTEXT_MENU_DETAIL: _, // If set, also indicates that the context menu is shown
  WINDOW_SET_POPUP_WINDOW_DETAIL: _, // If set, also indicates that the popup window is shown
+ WINDOW_TOGGLE_BOOKMARK_HANGER: _,
  WINDOW_SET_AUDIO_MUTED: _,
  WINDOW_SET_AUDIO_PLAYBACK_ACTIVE: _,
  WINDOW_SET_FAVICON: _,

js/contextMenus.js

@@ -50,7 +50,7 @@ const addBookmarkMenuItem = (label, siteDetail, closestDestinationDetail, isPare
  if (isParent) {
  siteDetail = siteDetail.set('parentFolderId', closestDestinationDetail && (closestDestinationDetail.get('folderId') || closestDestinationDetail.get('parentFolderId')))
  }
- windowActions.setBookmarkDetail(siteDetail, siteDetail, closestDestinationDetail)
+ windowActions.setBookmarkDetail(siteDetail, siteDetail, closestDestinationDetail, false)
  }
  }
 }
@@ -63,7 +63,7 @@ const addFolderMenuItem = (closestDestinationDetail, isParent) => {
  if (isParent) {
  emptyFolder = emptyFolder.set('parentFolderId', closestDestinationDetail && (closestDestinationDetail.get('folderId') || closestDestinationDetail.get('parentFolderId')))
  }
- windowActions.setBookmarkDetail(emptyFolder, undefined, closestDestinationDetail)
+ windowActions.setBookmarkDetail(emptyFolder, undefined, closestDestinationDetail, false)
  }
  }
 }
@@ -285,7 +285,7 @@ function siteDetailTemplateInit (siteDetail, activeFrame) {
  template.push(
  {
  label: locale.translation(isFolder ? 'editFolder' : 'editBookmark'),
- click: () => windowActions.setBookmarkDetail(siteDetail, siteDetail)
+ click: () => windowActions.setBookmarkDetail(siteDetail, siteDetail, null, true)
  },
  CommonMenu.separatorMenuItem)
  }