Improve UI/UX of bookmark popup

brave · Oct 13, 2016 · 8865ba7 · 8865ba7
1 parent 66774ae
commit 8865ba7
Show file tree

Hide file tree

Showing 12 changed files with 168 additions and 59 deletions.
diff --git a/app/extensions/brave/locales/en-US/app.properties b/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

diff --git a/app/extensions/brave/locales/en-US/menu.properties b/app/extensions/brave/locales/en-US/menu.properties
@@ -3,6 +3,7 @@ closeTab=Close
 closeOtherTabs=Close other Tabs
 bookmarkPage=Bookmark Page
 bookmarkLink=Bookmark This Link
+bookmarkAdded=Bookmark Added
 openFile=Open File…
 openLocation=Open Location…
 openSearch=Search for "{{selectedVariable}}"
@@ -19,7 +20,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 +52,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

diff --git a/docs/state.md b/docs/state.md
@@ -371,6 +371,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

diff --git a/docs/windowActions.md b/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) 

diff --git a/js/actions/windowActions.js b/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
     })
   },
 

diff --git a/js/components/addEditBookmark.js b/js/components/addEditBookmark.js
@@ -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()
   }
@@ -95,7 +100,7 @@ class AddEditBookmark extends ImmutableComponent {
     )
     appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, !isFirstBookmark || showBookmarksToolbar)
   }
-  onSave () {
+  onSave (closeDialog = true) {
     // First check if the title of the currentDetail is set
     if (!this.bookmarkNameValid) {
       return false
@@ -104,54 +109,67 @@ 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>
-          {
-            !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')} />
+    return <Dialog onHide={this.onClose} className='bookmarkDialog' isClickDismiss>
+      <div className='bookmarkForm' onClick={this.onClick}>
+        <div className='arrowUp' />
+        <div className='bookmarkFormInner'>
+          <h2 data-l10n-id='bookmarkAdded' />
+          <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>
-            : null
-          }
-          <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'>
             {
-              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>
   }

diff --git a/js/components/main.js b/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')
@@ -909,6 +908,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)}
@@ -993,14 +993,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}

diff --git a/js/components/navigationBar.js b/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
@@ -40,7 +41,7 @@ class NavigationBar extends ImmutableComponent {
   onToggleBookmark () {
     // 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, false)
   }
 
   onReload (e) {
@@ -73,6 +74,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,6 +114,15 @@ 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')} />
+        : null
+        }
       <div className='startButtons'>
         {
           isSourceAboutUrl(this.props.location) || this.titleMode
@@ -134,6 +145,14 @@ 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
+          })}
+          l10nId={this.bookmarked ? 'removeBookmarkButton' : 'addBookmarkButton'}
+          onClick={this.onToggleBookmark} />
       </div>
       <UrlBar ref='urlBar'
         sites={this.props.sites}
@@ -168,14 +187,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>

diff --git a/js/contextMenus.js b/js/contextMenus.js
@@ -49,7 +49,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)
     }
   }
 }
@@ -62,7 +62,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)
     }
   }
 }
@@ -278,7 +278,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)
     }

diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js
@@ -599,7 +599,8 @@ const doAction = (action) => {
         windowState = windowState.mergeIn(['bookmarkDetail'], {
           currentDetail: action.currentDetail,
           originalDetail: action.originalDetail,
-          destinationDetail: action.destinationDetail
+          destinationDetail: action.destinationDetail,
+          shouldShowLocation: action.shouldShowLocation
         })
       }
       // Since the input values of bookmarks are bound, we need to notify the controls sync.