Refactor popupWindow.less with Aphrodite #7929

luixxiul · 2017-03-28T08:39:18Z

Also add right:1em to create margin between the pop up and the window right border

Auditors: @bbondy @bridiver @bsclifton @cezaraugusto @jonathansampson - I would like to ask you to review the PR as you have edit popupWindow.js some way. Thanks in advance!

Test Plan:

Open about:preferences#security
Enable LastPass
Click the extension icon on the navigation bar
Make sure you can log in your account

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).

cezaraugusto · 2017-04-05T21:21:01Z

js/components/popupWindow.js

      }
    }

    return <div
      className={cx({
        popupWindow: true,
-        reverseExpand: styles.right !== undefined
+        [css(styles.popupWindow)]: true,
+        [css((style.right !== undefined) && styles.reverseExpand)]: true


I know it was here before but not sure what's the reason of having it? row-reverse does nothing here

jonathansampson · 2017-04-06T07:46:50Z

app/renderer/components/styles/global.js

@@ -129,6 +129,7 @@ const globalStyles = {
    switchNubShadow: '1px 1px 5px -2px black',
    buttonShadow: '0px 1px 5px -1px rgba(0, 0, 0, 1.0)',
    dialogShadow: '0px 8px 22px 0px rgba(0, 0, 0, .5)',
+    flyoutDialogBoxShadow: '2px 2px 8px #3b3b3b',


I don't think we should be using non-black hex colors for shadows. rgba handles this particular thing much better, IMHO.

Thanks for notifying. I'll replace it this with a variable later.

jonathansampson · 2017-04-06T07:46:50Z

js/components/popupWindow.js

@@ -69,35 +72,55 @@ class PopupWindow extends ImmutableComponent {
  }

  render () {
-    let styles = {}
+    let style = {}
    if (parseInt(this.width)) {


Can we avoid doing parseInt twice for this.width and this.height?

jonathansampson · 2017-04-06T07:46:50Z

js/components/popupWindow.js

    if (parseInt(this.width)) {
-      styles.width = (parseInt(this.width) + 2)
+      style.width = (parseInt(this.width) + 2)
    }
    if (parseInt(this.height)) {


Can we avoid doing parseInt twice for this.width and this.height?

++ on this comment

jonathansampson · 2017-04-06T07:46:50Z

js/components/popupWindow.js

+    padding: 0,
+    position: 'absolute',
+    zIndex: globalStyles.zindex.zindexPopupWindow,
+    WebkitUserSelect: 'none'


user-select (userSelect) is supported unprefixed in Blink. We should avoid using the prefixed version.

jonathansampson · 2017-04-06T07:46:50Z

less/variables.less

@@ -117,6 +117,7 @@

 @buttonShadow: 0px 1px 5px -1px rgba(0, 0, 0, 0.5);
 @dialogShadow: 0px 8px 22px 0px rgba(0, 0, 0, .5);
+@flyoutDialogBoxShadow: 2px 2px 8px #3b3b3b;


To echo my earlier comment; I don't think we should use non-black HEX colors for shadows. rgba handles this much better.

bsclifton

If you can address the feedback by @jonathansampson- the changes LGTM 😄

NejcZdovc

LGTM after @jonathansampson fixes

cezaraugusto · 2017-04-10T01:41:10Z

rebase pls

luixxiul · 2017-04-10T05:05:40Z

Would anyone please add another commit to fix the issues mentioned above about parseInt? Thanks!


        Refactor popupWindow.less with Aphrodite

Also add right:1em to create margin between the pop up and the window right border Closes #7927 Auditors: Test Plan: 1. Open about:preferences#security 2. Enable LastPass 3. Click the extension icon on the navigation bar 4. Make sure you can log in your account

@jonathansampson


        update per @jonathansampson feedback

@jonathansampson

Auditors: @jonathansampson @luixxiul


        removes unecessary syles for popupWindow

Auditors: @luxxiul

cezaraugusto

rebased, removed remaining LESS files and updated per @jonathansampson feedback. Waiting others feedback, ++ from me

jonathansampson · 2017-04-17T17:37:32Z

js/components/popupWindow.js

-      if (this.top + this.height < window.innerHeight) {
-        style.top = this.top
+    if (top) {
+      if (top + this.height < window.innerHeight) {


I think you meant top + height here.


        Improve code readability

@jonathansampson

Auditors: @jonathansampson

luixxiul · 2017-04-17T19:27:20Z

thanks guys for helping me out :-)

luixxiul added feature/extensions PR/needs-QA-attention ☕ refactoring refactoring/aphrodite labels Mar 28, 2017

luixxiul added this to the 0.14.1 milestone Mar 28, 2017

luixxiul self-assigned this Mar 28, 2017

luixxiul requested review from bridiver, jonathansampson, bbondy, cezaraugusto and bsclifton Mar 28, 2017

luixxiul mentioned this pull request Apr 3, 2017

Refactor popupWindow.less with Aphrodite #7927

Closed

cezaraugusto reviewed Apr 5, 2017

View changes

jonathansampson suggested changes Apr 6, 2017

View changes

luixxiul requested review from NejcZdovc and removed request for bridiver and bbondy Apr 6, 2017

bsclifton approved these changes Apr 7, 2017

View changes

NejcZdovc approved these changes Apr 7, 2017

View changes

luixxiul modified the milestones: 0.14.3, 0.14.2 Apr 9, 2017

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

luixxiul added help wanted and removed PR/needs-rebase ۞ labels Apr 10, 2017

Suguru Hirahara and others added 3 commits Apr 10, 2017

update per @jonathansampson feedback

Verified

This commit was signed with a verified signature.

cezaraugusto Cezar Augusto

GPG key ID: ECC5FF1C7EA1D39D Learn about signing commits

304ae64

Auditors: @jonathansampson @luixxiul

removes unecessary syles for popupWindow

Verified

This commit was signed with a verified signature.

cezaraugusto Cezar Augusto

GPG key ID: ECC5FF1C7EA1D39D Learn about signing commits

Loading status checks…

03533b3

Auditors: @luxxiul

cezaraugusto approved these changes Apr 16, 2017

View changes

luixxiul removed the help wanted label Apr 16, 2017

jonathansampson suggested changes Apr 17, 2017

View changes

Improve code readability

Verified

This commit was signed with a verified signature.

cezaraugusto Cezar Augusto

GPG key ID: ECC5FF1C7EA1D39D Learn about signing commits

Loading status checks…

e299f85

Auditors: @jonathansampson

jonathansampson approved these changes Apr 17, 2017

View changes

cezaraugusto merged commit 0d6d9b5 into brave:master Apr 17, 2017
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build failed
Details

luixxiul deleted the luixxiul:popupWindow-aphrodite branch Apr 17, 2017

luixxiul removed the PR/needs-QA-attention ☕ label Apr 17, 2017

brave / browser-laptop Archived

Refactor popupWindow.less with Aphrodite #7929

Refactor popupWindow.less with Aphrodite #7929

luixxiul commented Mar 28, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bsclifton left a comment

NejcZdovc left a comment

cezaraugusto commented Apr 10, 2017

luixxiul commented Apr 10, 2017

cezaraugusto left a comment •

edited

This comment has been minimized.

luixxiul commented Apr 17, 2017

brave / browser-laptop Archived

Join GitHub today

Refactor popupWindow.less with Aphrodite #7929

Refactor popupWindow.less with Aphrodite #7929

Conversation

luixxiul commented Mar 28, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bsclifton left a comment

NejcZdovc left a comment

cezaraugusto commented Apr 10, 2017

luixxiul commented Apr 10, 2017

cezaraugusto left a comment • edited

This comment has been minimized.

luixxiul commented Apr 17, 2017

cezaraugusto left a comment •

edited