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:38:06Z

js/about/extensions.js

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


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

cezaraugusto · 2017-02-23T18:41:14Z

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:41:36Z

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:41:58Z

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:43:35Z

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:50:42Z

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:30:31Z

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:32:07Z

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:32:24Z

js/about/extensions.js

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


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

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 February 21, 2017 19:09

luixxiul commented Feb 21, 2017

View reviewed 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 reviewed 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 reviewed changes

bsclifton approved these changes Mar 13, 2017

View reviewed 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

luixxiul deleted the aboutExtensions-aphrodite branch March 13, 2017 05:11

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor about:extensions with Aphrodite #7346

Refactor about:extensions with Aphrodite #7346

luixxiul commented Feb 21, 2017 •

edited by bsclifton

Loading

luixxiul Feb 21, 2017

cezaraugusto Feb 23, 2017

bsclifton Mar 12, 2017

cezaraugusto Feb 23, 2017

cezaraugusto Feb 23, 2017

cezaraugusto Feb 23, 2017

cezaraugusto Feb 23, 2017

cezaraugusto Feb 23, 2017

luixxiul Feb 24, 2017 •

edited

Loading

bsclifton Mar 12, 2017

cezaraugusto Feb 23, 2017

bsclifton commented Mar 10, 2017

luixxiul commented Mar 11, 2017

bsclifton left a comment

bsclifton Mar 12, 2017

bsclifton Mar 12, 2017

luixxiul Mar 13, 2017 •

edited

Loading

bsclifton Mar 13, 2017

bsclifton Mar 12, 2017

luixxiul commented Mar 13, 2017

bsclifton left a comment

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

		const styles = StyleSheet.create({

Refactor about:extensions with Aphrodite #7346

Refactor about:extensions with Aphrodite #7346

Conversation

luixxiul commented Feb 21, 2017 • edited by bsclifton Loading

Test Plan

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Mar 10, 2017

luixxiul commented Mar 11, 2017

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul commented Mar 13, 2017

bsclifton left a comment

Choose a reason for hiding this comment

luixxiul commented Feb 21, 2017 •

edited by bsclifton

Loading

luixxiul Feb 24, 2017 •

edited

Loading

luixxiul Mar 13, 2017 •

edited

Loading