Refactor about:extensions with Aphrodite #7346

luixxiul · 2017-02-21T19:09:20Z

Test Plan

Open about:preferences#advanced
Disable Torrent Viewer
Restart the browser
Open about:extensions
Make sure Torrent Viewer is disabled (grayed out)
Enable Torrent Viewer
Make sure Torrent Viewer is enabled

Description

Closes #7345

extensions.less is deleted
itemList.less is unlinked
globalStyles.spacing.aboutPageDetailsPageWidth is introduced

Auditors: @cezaraugusto, @bsclifton

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

~~Blocked on #7258~~ #7258 was merged to the master

luixxiul · 2017-02-21T19:09:46Z

js/about/extensions.js

  render () {
+    const className = css(
+      commonStyles.listItem,


cf: https://github.com/brave/browser-laptop/pull/7258/files#diff-fa59d1e0221a5bee94fd2abd9b96dcfd

+++++++++++++++

+++++++++++++++++

cezaraugusto · 2017-02-23T18:53:07Z

js/about/extensions.js

  render () {
+    const className = css(
+      commonStyles.listItem,


+++++++++++++++

cezaraugusto · 2017-02-23T18:53:07Z

js/about/extensions.js

-      <div className='extensionImage'>
-        <img src={`${this.props.extension.get('base_path')}/${icon}`} />
+      <div className={css(styles.extensionImage)}>
+        <img className={css(styles.img)} src={`${this.props.extension.get('base_path')}/${icon}`} />
      </div>
      <div className='extensionDetails'>


can we remove that class or just replace it to data-id-test just for future-friendliness?

cezaraugusto · 2017-02-23T18:53:07Z

js/about/extensions.js

@@ -113,7 +158,7 @@ class AboutExtensions extends React.Component {
    })
  }
  render () {
-    return <div className='extensionDetailsPage'>
+    return <div className={css(styles.detailsPage)}>
      <h2 data-l10n-id='extensions' />
      <div className='extensionDetailsPageContent'>


cezaraugusto · 2017-02-23T18:53:07Z

js/about/extensions.js

-        <h3 className='extensionTitle'>{bravifyText(this.props.extension.get('name'))}</h3>
-        <span className='extensionVersion'>{this.props.extension.get('version')}</span>
+        <h3 className={css(styles.extensionTitle)}>{bravifyText(this.props.extension.get('name'))}</h3>
+        <span className={css(styles.extensionVersion)}>{this.props.extension.get('version')}</span>
        {
          !['__MSG_extDescriptionGoogleChrome__', '__MSG_appDesc__'].includes(this.props.extension.get('description'))
          ? <div className='extensionDescription'>{bravifyText(this.props.extension.get('description'))}</div>


cezaraugusto · 2017-02-23T18:53:07Z

js/about/extensions.js

 require('../../node_modules/font-awesome/css/font-awesome.css')

+const styles = StyleSheet.create({


[minor] I think it's more likely to edit logic code more than editing styles, and this way we have to scroll down all Aphrodite stuff to get there. Can we put styles at the end of the file?

I put that here bearing in mind that on HTML files inlined styles are placed before body tag. Moving these to the bottom is fine for me too. CC @bbondy and @bsclifton for comments. I think we do not have the rule on the placement (Sorry if I missed it).

Moving to the bottom would be great (so that everything is consistent) 👍

cezaraugusto · 2017-02-23T18:53:07Z

less/about/extensions.less

-      opacity: 0.3;
-    }
-  }
-}


💯

"let's use less LESS"

bsclifton · 2017-03-10T22:07:52Z

@luixxiul let me know once you address the feedback and I can review / merge 😄

luixxiul · 2017-03-11T14:12:30Z

Ok updated. Let me know if it is ok so I will squash the commits ;-)

bsclifton

Comments left- otherwise, looks great! 😄 Once you've implemented feedback and squashed, we're good to go on merge. I made sure the about:extensions tests pass (vimium fails, but that's expected) and all unit tests pass 👍

bsclifton · 2017-03-12T09:34:12Z

js/about/extensions.js

 require('../../node_modules/font-awesome/css/font-awesome.css')

+const styles = StyleSheet.create({


Moving to the bottom would be great (so that everything is consistent) 👍

bsclifton · 2017-03-12T09:34:12Z

app/renderer/components/styles/global.js

@@ -106,6 +106,7 @@ const globalStyles = {
    navbarBraveButtonMarginLeft: '80px',
    navbarLeftMarginDarwin: '76px',
    sideBarWidth: '190px',
+    aboutPageDetailsPageWidth: '704px',


this variable name is not very clear... Can we name it something more appropriate, like aboutExtensionDetailWidth?

I think I named so in order to replace other "704px"s on about pages with that variable (this one too: 9ad4b84#diff-9d96fcb58c1eeedf8e3d6d7f46b1a794R118).

Is it OK to do so with this PR or another one?

sure, that works 👍

bsclifton · 2017-03-12T09:34:12Z

js/about/extensions.js

  render () {
+    const className = css(
+      commonStyles.listItem,


+++++++++++++++++


        Refactor about:extensions with Aphrodite

Closes #7345 - extensions.less is deleted - itemList.less is unlinked - globalStyles.spacing.aboutPageDetailsPageWidth is introduced Auditors: Test Plan: 1. Open about:preferences#advanced 2. Disable Torrent Viewer 3. Restart the browser 4. Open about:extensions 5. Make sure Torrent Viewer is disabled (grayed out) 6. Enable Torrent Viewer 7. Make sure Torrent Viewer is enabled

luixxiul · 2017-03-13T04:43:26Z

Squashed!

bsclifton

++

luixxiul added feature/about-pages PR/blocked PR/needs-QA-attention ☕ refactoring/aphrodite labels Feb 21, 2017

luixxiul requested review from cezaraugusto and bsclifton Feb 21, 2017

luixxiul reviewed Feb 21, 2017

View changes

luixxiul mentioned this pull request Feb 21, 2017

Refactor about:extensions with Aphrodite #7345

Closed

luixxiul removed the PR/blocked label Feb 23, 2017

cezaraugusto reviewed Feb 23, 2017

View changes

bsclifton assigned luixxiul Mar 10, 2017

bsclifton added PR/work-in-progress ⚒ and removed PR/needs-QA-attention ☕ labels Mar 11, 2017

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

luixxiul added this to the 0.13.6 milestone Mar 11, 2017

bsclifton suggested changes Mar 12, 2017

View changes

bsclifton approved these changes Mar 13, 2017

View changes

bsclifton added QA/test-plan-specified release-notes/include and removed PR/needs-QA-attention ☕ labels Mar 13, 2017

bsclifton merged commit 14cd6ae into brave:master Mar 13, 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 is in progress
Details

luixxiul deleted the luixxiul:aboutExtensions-aphrodite branch Mar 13, 2017

luixxiul removed QA/test-plan-specified release-notes/include labels Mar 15, 2017

brave / browser-laptop Archived

Refactor about:extensions with Aphrodite #7346

Refactor about:extensions with Aphrodite #7346

luixxiul commented Feb 21, 2017 •

edited by bsclifton

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.

This comment has been minimized.

This comment has been minimized.

bsclifton commented Mar 10, 2017

luixxiul commented Mar 11, 2017

bsclifton left a comment

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.

luixxiul commented Mar 13, 2017

bsclifton left a comment

		require('../../node_modules/font-awesome/css/font-awesome.css')

		const styles = StyleSheet.create({

brave / browser-laptop Archived

Join GitHub today

Refactor about:extensions with Aphrodite #7346

Refactor about:extensions with Aphrodite #7346

Conversation

luixxiul commented Feb 21, 2017 • edited by bsclifton

Test Plan

Description

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.

luixxiul Feb 24, 2017 • edited

This comment has been minimized.

This comment has been minimized.

bsclifton commented Mar 10, 2017

luixxiul commented Mar 11, 2017

bsclifton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

luixxiul Mar 13, 2017 • edited

This comment has been minimized.

This comment has been minimized.

luixxiul commented Mar 13, 2017

bsclifton left a comment

luixxiul commented Feb 21, 2017 •

edited by bsclifton

luixxiul Feb 24, 2017 •

edited

luixxiul Mar 13, 2017 •

edited