Refactor addEditBookmarkHanger.js with commonForm #8136

luixxiul · 2017-04-09T05:23:51Z

Closes #6040
Addresses #7989 #8010

Added CommonFormBookmarkHanger to commonForm.js
Added globalStyles.spacing.bookmarkHangerMaxWidth to global.js
Max-width of the bookmark hanger was set to 350px. Also, setting white-space:normal makes it possible for lines to be wrapped.
Applied Dialog to addEditBookmarkHanger.js instead of addEditBookmark.js
Added cancel button with this.onClose as 'fa fa-close' was removed
Added const arrowUp and dialogHanger
Added styles.bookmarkHanger which is applied if !this.props.isModal
Copied CommonFormSection and CommonFormTitle from commonForm.js, adding CommonFormBookmarkHangerTitle to addEditBookmarkHanger.js
The margin-top of the title should be reduced if this.props.isModal.
Changed backgroundColor of commonForm to #fff
Add zindex to elements on navigation bar to let tests pass
The value was set to higher than that of dialog, which is 3000.
Updated test code

Also:

Replaced divs with sections
Removed styles from form.less
Removed wideButton from buttons
Added testIds to buttons

Auditors:

Test Plan 1:

Open about:preferences
Disable Home button
Click the star icon on the URL bar
Make sure the arrow up appears exactly under the star
Open about:preferences
Enable Home button
Click the star icon again
Make sure th arrow up appears exactly under the star

Test Plan 2:

Open about:bookmarks
Click the star icon on the URL bar
Click "Done"
Click the star icon again
Click "Remove"
Make sure the bookmark is successfully removed

Test Plan 3:

Click "Add Folder" icon on about:bookmarks
Click "Cancel"
Make sure the dialog is closed

Test Plan 4:

Create a bookmark folder on about:bookmarks
Edit the folder via context menu on about:bookmarks
Click "Remove"
Make sure the folder is successfully removed

Test Plan 5:

Click the star icon on about:bookmarks
Click "Cancel"
Make sure the dialog is closed
Click the star icon again
Create a bookmark
Edit the bookmark via context menu
Click "Remove"
Make sure the bookmark is successfully removed

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Test Plan:

luixxiul · 2017-04-09T05:27:35Z

Test plan 1

Test plan 2

Test plan 3

Test plan 4

Test plan 5

bradleyrichter · 2017-04-17T16:53:45Z

@luixxiul Can you please find a way to add shadow to the triangle nub as part of this refactor? It is barely visible in it's current form.

cezaraugusto · 2017-04-17T17:52:47Z

rebase pls

luixxiul · 2017-04-18T14:13:19Z

@bradleyrichter I updated the arrow with box-shadow.

bradleyrichter

looks much better. thanks!

Please also move the window up so the point of the nub is between the legs of the arrow button, similar to chrome.

luixxiul · 2017-04-18T16:43:38Z

Please also move the window up so the point of the nub is between the legs of the arrow button, similar to chrome.

I tried but I cannot find an easy way of fixing it without changing default behavior (ie clicking the star button twice displays "edit bookmark" dialog); the fix should be related with z-index, which is out of scope of this PR, so I think it is a good idea to deal with the issue with another PR.

Without changing z-index you'll get this:

cezaraugusto

Changes looks great, so do tests, nice job here.

left some comments regarding naming, sorry if I'm being picky but it's a step forward standardization

cezaraugusto · 2017-04-18T20:52:11Z

app/renderer/components/addEditBookmarkHanger.js

+        })} data-l10n-id={this.heading} />
+        <CommonFormSection>
+          <div className={css(styles.bookmark__sectionWrapper)}>
+            <section>


++ for section

cezaraugusto · 2017-04-18T20:55:55Z

app/renderer/components/addEditBookmarkHanger.js

  }
 }

+const styles = StyleSheet.create({
+  // Copied from commonForm.js
+  CommonFormSection: {


can we use camelCase here?

cezaraugusto · 2017-04-18T20:56:09Z

app/renderer/components/addEditBookmarkHanger.js

+    // PR #7985
+    margin: `${globalStyles.spacing.dialogInsideMargin} 30px`
+  },
+  CommonFormTitle: {


cezaraugusto · 2017-04-18T20:56:55Z

app/renderer/components/commonForm.js

@@ -98,6 +108,13 @@ const styles = StyleSheet.create({
    maxWidth: globalStyles.spacing.dialogLargeWidth
  },

+  CommonFormBookmarkHanger: {


luixxiul · 2017-04-19T07:25:18Z

@cezaraugusto thanks for reviewing. I just missed the camel case stuff, will fix it later.

luixxiul · 2017-04-19T14:22:47Z

@cezaraugusto ready for re-review. If it looks good to you, please set ready-to-merge label. I will then squash the commits to merge the test plans and sign the commit with GPG, thanks!

Closes #6040 Addresses #7989 - Added CommonFormBookmarkHanger to commonForm.js - Added globalStyles.spacing.bookmarkHangerMaxWidth to global.js Max-width of the bookmark hanger was set to 350px. Also, setting white-space:normal makes it possible for lines to be wrapped. - Moved Dialog from addEditBookmark.js to addEditBookmarkHanger.js - Added cancel button with this.onClose as 'fa fa-close' was removed - Added const arrowUp and dialogHanger - Added box-shadow to arrowUp - Added styles.bookmarkHanger which is applied if !this.props.isModal - Copied CommonFormSection and CommonFormTitle from commonForm.js, adding CommonFormBookmarkHangerTitle to addEditBookmarkHanger.js The margin-top of the title should be reduced if this.props.isModal. - Changed backgroundColor of commonForm to #fff - Add zindex to elements on navigation bar to let tests pass The value was set to higher than that of dialog, which is 3000. - Updated test code Also: - Replaced divs with sections - Removed styles from form.less - Removed wideButton from buttons - Added testIds to buttons (bookmarkHangerRemoveButton, bookmarkHangerCancelButton, bookmarkHangerDoneButton) - Fixed camel cases Auditors: Test Plan 1: 1. Open about:preferences 2. Disable Home button 3. Click the star icon on the URL bar 4. Make sure the arrow up appears exactly under the star 5. Make sure the button appears under the icon 6. Open about:preferences 7. Enable Home button 8. Click the star icon again 9. Make sure th arrow up appears exactly under the star Test Plan 2: 1. Open about:bookmarks 2. Click the star icon on the URL bar 3. Click "Done" 4. Click the star icon again 5. Click "Remove" 6. Make sure the bookmark is successfully removed Test Plan 3: 1. Click "Add Folder" icon on about:bookmarks 2. Click "Cancel" 3. Make sure the dialog is closed Test Plan 4: 1. Create a bookmark folder on about:bookmarks 2. Edit the folder via context menu on about:bookmarks 3. Click "Remove" 4. Make sure the folder is successfully removed Test Plan 5: 1. Click the star icon on about:bookmarks 2. Click "Cancel" 3. Make sure the dialog is closed 4. Click the star icon again 5. Create a bookmark 6. Edit the bookmark via context menu 7. Click "Remove" 8. Make sure the bookmark is successfully removed

luixxiul added feature/bookmarks PR/needs-QA-attention ☕ refactoring labels Apr 9, 2017

luixxiul self-assigned this Apr 9, 2017

luixxiul requested a review from NejcZdovc April 9, 2017 05:23

luixxiul mentioned this pull request Apr 9, 2017

Apply commonForm to addEditBookmarkHanger.js #6040

Closed

luixxiul requested review from cezaraugusto and bsclifton April 10, 2017 14:49

luixxiul added this to the 0.14.3 milestone Apr 10, 2017

luixxiul mentioned this pull request Apr 16, 2017

Add ability to create a new bookmark folder in BM hanger flow #8346

Closed

NejcZdovc mentioned this pull request Apr 17, 2017

Move files into app folder (Bookmarks) #8351

Closed

cezaraugusto added the PR/needs-rebase ۞ label Apr 17, 2017

luixxiul added PR/work-in-progress ⚒ and removed PR/needs-QA-attention ☕ labels Apr 17, 2017

luixxiul added PR/needs-QA-attention ☕ and removed PR/needs-rebase ۞ PR/work-in-progress ⚒ labels Apr 18, 2017

luixxiul requested review from bradleyrichter and removed request for NejcZdovc and bsclifton April 18, 2017 14:19

bradleyrichter approved these changes Apr 18, 2017

View reviewed changes

cezaraugusto reviewed Apr 18, 2017

View reviewed changes

luixxiul force-pushed the refactoring-aphrodite branch from 66c6f33 to a278055 Compare April 19, 2017 14:08

luixxiul mentioned this pull request Apr 19, 2017

Refactor clearBrowsingDataPanel.js with commonForm #8194

Merged

4 tasks

cezaraugusto added PR/ready-for-merge and removed PR/needs-QA-attention ☕ labels Apr 19, 2017

luixxiul merged commit 192d625 into brave:refactoring-aphrodite Apr 20, 2017

luixxiul removed the PR/ready-for-merge label Apr 20, 2017

This was referenced Apr 21, 2017

Discard .genericForm and .commonForm from forms.less #8010

Closed

Delete genericForm and commonDialog from forms.less #8155

Merged

Refactor components with commonForm #8443

Merged

luixxiul deleted the addEditBookmarkHanger-refactoring branch April 25, 2017 01:17

This was referenced Apr 26, 2017

Replace input and select elements with <FormTextbox> and <FormDropdown> in .bookmarkFormTable #6777

Closed

remove wideButton class from the buttons on the Edit Bookmark dialog #6400

Closed

bookmark modal animation is incredibly slow #6748

Closed

luixxiul mentioned this pull request May 6, 2017

the bookmark hanger being faded in #6380

Closed

luixxiul mentioned this pull request May 28, 2017

arrowup on bookmarkhanger is misaligned #6399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor addEditBookmarkHanger.js with commonForm #8136

Refactor addEditBookmarkHanger.js with commonForm #8136

luixxiul commented Apr 9, 2017 •

edited

Loading

luixxiul commented Apr 9, 2017 •

edited

Loading

bradleyrichter commented Apr 17, 2017

cezaraugusto commented Apr 17, 2017

luixxiul commented Apr 18, 2017

bradleyrichter left a comment

luixxiul commented Apr 18, 2017

cezaraugusto left a comment

cezaraugusto Apr 18, 2017

cezaraugusto Apr 18, 2017

cezaraugusto Apr 18, 2017

cezaraugusto Apr 18, 2017

luixxiul commented Apr 19, 2017

luixxiul commented Apr 19, 2017

Refactor addEditBookmarkHanger.js with commonForm #8136

Refactor addEditBookmarkHanger.js with commonForm #8136

Conversation

luixxiul commented Apr 9, 2017 • edited Loading

luixxiul commented Apr 9, 2017 • edited Loading

Test plan 1

Test plan 2

Test plan 3

Test plan 4

Test plan 5

bradleyrichter commented Apr 17, 2017

cezaraugusto commented Apr 17, 2017

luixxiul commented Apr 18, 2017

bradleyrichter left a comment

Choose a reason for hiding this comment

luixxiul commented Apr 18, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

cezaraugusto Apr 18, 2017

Choose a reason for hiding this comment

cezaraugusto Apr 18, 2017

Choose a reason for hiding this comment

cezaraugusto Apr 18, 2017

Choose a reason for hiding this comment

cezaraugusto Apr 18, 2017

Choose a reason for hiding this comment

luixxiul commented Apr 19, 2017

luixxiul commented Apr 19, 2017

luixxiul commented Apr 9, 2017 •

edited

Loading

luixxiul commented Apr 9, 2017 •

edited

Loading