Create sectionTitle.js #8237

luixxiul · 2017-04-11T19:00:11Z

TODO: Refactor modalOverlay.js to remove .sectionTitle.

Closes #7750
Closes #8165
Closes #8234
Addresses #8235
Closes #8349
Closes #8350

1. Created `TitleWrapper`, `SectionTitle`, and `SubTitle`

SectionTitleWrapper and SectionTitleLabelWrapper
DefaultSectionTitle and AboutPageSectionTitle
AboutPageSectionSubTitle and SectionLabelTitle
Copied some properties from common.less to sectionTitle.js

Note: Visit about:styles for their templates.

2. Refactored with those components:

extensionsTab.js, pluginsTab.js, preferences.js, syncTab.js, and paymentsTab.js
about.js, brave.js, bookmarks.js and history.js
torrentViewer.js, torrentStatus.js and torrentFileList.js

3. Added `siteDetailsPageContent` to commonStyles.js and applied it to:

about.js, brave.js, and history.js

4. Removed styles of `sectionTitle` from:

brave.less
siteDetails.less
preferences.less

Note: Use the components of sectionTitle instead of <div class='sectionTitle'>

Also:

Moved sectionTitle.js into the common folder
Added TOC to about:styles with internal links
Removed CommonFormSubSection from importBrowserDataPanel.js

Test Plans

Test Plan 1:

Open about:preferences#search
Make sure there is margin under the top header

Test Plan 2:

Open about:preferences#sync
Make sure "beta" is aligned top
Open about:preferences#payments
Make sure the labels appear the same row
Make sure the switches and the labels are aligned exactly like Restructuring paymentsTab.js #7751

Test Plan 3:

Open about:about
Make sure there is margin under the header
Open about:brave, about:bookmarks, and about:history
Make sure the header font size is same

Test Plan 4:

Open https://webtorrent.io/free-torrents/
Click .torrent link
Make sure the header font size is same as about:brave
Start download
Make sure the header font size is same

Test Plan 5:

Clear your profile folder
Open about:preferences#sync
Enable Sync
Make sure there is margin under "Devices"

Test Plan 6:

Open "Import Browser data" from the menu
Make sure the switch is indented

Test Plan 7:

Open about:styles
Click "commonForm" on TOC
Make sure the template of the commonForm is displayed
Click "Go back to top"
Click "sectionTitle" on TOC
Make sure section title components are displayed

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-17T10:46:14Z

app/renderer/components/preferences/paymentsTab.js

@@ -264,8 +276,7 @@ const styles = StyleSheet.create({
  paymentsContainer: {
    position: 'relative',
    overflowX: 'hidden',
-    width: '805px',
-    marginTop: '15px'


To make the header of payments and sync appear on the same height.

luixxiul · 2017-04-17T10:53:12Z

app/renderer/components/sectionTitle.js

+  }
+}
+
+class PrefSectionTitle extends ImmutableComponent {


As this is not going to be used, I'm removing it.

luixxiul · 2017-04-17T10:54:14Z

js/webtorrent/components/torrentViewer.js

@@ -9,6 +9,8 @@ const Button = require('../../components/button')
 const TorrentFileList = require('./torrentFileList')
 const TorrentStatus = require('./torrentStatus')

+const {AboutPagesSectionTitle} = require('../../../app/renderer/components/sectionTitle')


I'll rename the component (webtorrent is not one of about pages)

NejcZdovc · 2017-04-17T11:18:49Z

app/renderer/components/sectionTitle.js

+const globalStyles = require('./styles/global')
+const commonStyles = require('./styles/commonStyles')
+
+class SectionTitleWrapper extends ImmutableComponent {


Please split this into single component per file. You should create new folder and place everything there. Name of file == component name, for this example sectionTitleWrapper

I'm afraid doing so would make it hard to maintain the style consistency and enhance components (since you would need to check out every file in the folder not to create duplicates). For me it could cause style scattering.

Actually I prefer calling components with one line like this: https://github.com/brave/browser-laptop/pull/8237/files#diff-943709aef6ee258bb12a2cbb33018988R18, which is the same way as you would load <commonForm> components. See: https://github.com/brave/browser-laptop/pull/8011/files#diff-fd618a0193d7534a298420a3d3b4a711R22

I'm cool with splitting anyway. I just would like to know how to avoid styles and components from scattering. (adding components to about:styles would be that way.)

yeah what I would do is add folder for it. This way we know that all style components for about pages are there. Problem with multiple components in one file is that it's really hard to know what you have.

Fine, still let's make sure to add the components to about:styles (as we do for textbox.js and dropdown.js for example). Splitting and adding them would be perfect.

We could create a sub panel on about:styles for components in the same folder. Creating a long one page is not what we would like to see.

what if we create new folder in renderer folder named styles and we place all styling components into it (one component per file). In there we would keep the same folder structure as we have in components folder. This way we keep the context of it. What do you think?

This is one option. Another one is to make exception for styling components and keep them in one file (I still prefer splitting them)

cc @bbondy @bsclifton @cezaraugusto

Regarding including, I also like this destructured approach. What I would suggest is something like this.

You are now creating things for sectionTitle. Create folder sectionTitle and place all components into it. Then in this folder create file index.js where you are exporting all this components. This way you can easily use your approach and file structure is still search and time friendly 😃

Based on this PR I will crate wiki regarding best practices, so that we will all work in the same way. So this PR is really important, we should define and think things in detail. Please don't change things yet until we find and define how should we tackle this one.

Sweet, because I'm still a fan of about:styles, linking to that file from about:styles looks good finally, in my opinion.

Waiting for opinion of other members who will be back online after the holiday.

I endorse creating a new folder to include this file but despite being enthusiastic on creating small components, they're still too small to host their own files IMO.

Since they're just titles, it's very unlikely that we'll add more methods and bulk file with new code.

For the new folder name, I'd suggest something like common/shared to host this, and in future include things like button and our already existent settings component, so for all highly replicated components. Even if we decide to create an about folder -- which I endorse as well -- they should be on that folder

luixxiul · 2017-04-17T11:41:37Z

less/modalOverlay.less

@@ -77,6 +77,7 @@
      transition: @transitionFast;
    }

+    /* Copied to siteDetails.js */


Will correct the comment -> sectionTitle

cezaraugusto

@luixxiul mind just moving sectionTitle inside a folder called common i.e. app/renderer/components/common/sectionTitle? We'd better separate concerns for now for better structure while we are not done refactoring everything.

I'm doing button refactoring and will be moving that as well once is done.

@NejcZdovc are you ok with that for now? If just in case file gets too big we can split that (as you did for tabContent) but I don't think that's the case for now

luixxiul · 2017-04-19T01:24:17Z

mind just moving sectionTitle inside a folder called common i.e. app/renderer/components/common/sectionTitle?

it sound good to me; we would then move other files (dropdown.js, list.js, textbox.js and messageBox.js) into that folder as well.

NejcZdovc · 2017-04-19T05:45:08Z

@cezaraugusto as I said I am partly ok with styling component's to be in the same folder, because I think that we won't write test for them. I would still prefer separate files, but we should only use this approach for styling components and nothing more. All other components should be in separate files.

luixxiul · 2017-04-19T06:23:29Z

It seems to me that it is not good timing to separate everything rigorously. We even do not know yet how many files and components there will be after refactor work, and how kind of issues there will be coming due to either "many files with a single component" or "many components with a single file". It is not a good idea to spend time on the issue which is yet to exist.

So, how about trying both way to create rules later? I don't think there is the general solution here (as there is not so much projects other than us who adopt Aphrodite), and both way seems to me worth trying to find the better one.

To avoid the chaos, we should create a tracking issue, commenting which components are refactored in which way.

On secitonTitle.js, let me add the sectionTitle components to about:styles precisely and see if that will avoid the issue.

NejcZdovc · 2017-04-19T09:41:35Z

app/renderer/components/sectionTitle.js

+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const React = require('react')
+const ImmutableComponent = require('../../../js/components/immutableComponent')


ImmutableComponent was moved into app folder, please update this path

@luixxiul please update this path

cezaraugusto · 2017-04-19T16:50:20Z

@NejcZdovc sorry if you got me wrong, I agree with you. I just think that for this case, specifically, we should leave as-is. Those titles don't have conditional rendering, nor do anything dynamically that's worth testing. The benefit of having them all together is the easy to change quickly if design spec changes

bsclifton · 2017-04-19T18:27:04Z

@NejcZdovc @cezaraugusto @luixxiul I saw the above (conversation about breaking out the components 1 per file)- it looks OK to me as-is

Because there are multiple in one file and they are related, it's easy to edit there. What pops out to me is that none of the components have logic, other than render. Since they are related and ONLY have a render, I would argue it's OK to have them in the same file 😄 Once a component adds any other property or becomes unrelated to the ones there, I believe it should be moved out. Just my 2 cents 😛

An example where I myself did this is with the TextBox component:
https://github.com/brave/browser-laptop/blob/master/app/renderer/components/textbox.js

What do you all think?

bsclifton

LGTM... but let's make sure @cezaraugusto and @NejcZdovc approve of the changes before merging 😄

cezaraugusto

I would just put titles inside a common folder as discussed, there's an awesome work on redux refactoring so it's a good time to endorse the separation of concerns.

Since we all agree on that, once done LGTM

NejcZdovc

in sectionTitle.js path needs to be updated for an immutable component

NejcZdovc · 2017-04-21T09:58:52Z

@luixxiul pleased check tests as well, because we have some failing tests https://travis-ci.org/brave/browser-laptop/builds/224193912

luixxiul · 2017-04-21T10:18:17Z

I think I moved the file to common folder but the commit was dropped accidentally.. I'll fix it soon, including the tests failure

Closes #7750 Closes #8165 Closes #8234 Addresses #8235 TODO: Refactor modalOverlay.js to remove .sectionTitle Created TitleWrapper, SectionTitle, and SubTitle - SectionTitleWrapper and BetaSectionTitleWrapper - DefaultSectionTitle, PrefSectionTitle, and AboutPagesSectionTitle - AboutPagesSectionSubTitle and SectionSubTitle Refactored with those components: - extensionsTab.js, pluginsTab.js, preferences.js, syncTab.js, and paymentsTab.js - about.js, brave.js, bookmarks.js and history.js - torrentViewer.js, torrentStatus.js and torrentFileList.js Added siteDetailsPageContent to commonStyles.js and applied: - about.js, brave.js, and history.js Removed styles of sectionTitle from: - brave.less - siteDetails.less Auditors: Test Plan 1: 1. Open about:preferences#search 2. Make sure there is margin under the top header Test Plan 2: 1. Open about:preferences#sync 2. Make sure "beta" is aligned top 3. Open about:preferences#payments 4. Make sure the labels appear the same row 5. Make sure the switches and the labels are aligned exactly like #7751 Test Plan 3: 1. Open about:about 2. Make sure there is margin under the header 3. Open about:brave, about:bookmarks, and about:history 4. Make sure the header font size is same Test Plan 4: 1. Open https://webtorrent.io/free-torrents/ 2. Click .torrent link 3. Make sure the header font size is same as about:brave 4. Start download 5. Make sure the header font size is same Test Plan 5: 1. Clear your profile folder 2. Open about:preferences#sync 3. Enable Sync 4. Make sure there is margin under "Devices"

Closes #8349 Closes #8350 Also: - Moved sectionTitle.js into the common folder - Added TOC to about:styles with internal links - Removed CommonFormSubSection from importBrowserDataPanel.js - Removed PrefSectionTitle - Renamed SectionSubTitle into SectionLabelTitle - Renamed BetaSectionTitleWrapper into SectionTitleLabelWrapper - Renamed SectionSubTitle into SectionLabelTitle - Renamed AboutPagesSectionTitle to AboutPageSectionTitle - Renamed AboutPagesSectionSubTitle to AboutPageSectionSubTitle - Copied some properties from common.less to sectionTitle.js Auditors: Test Plan 1: 1. Open "Import Browser data" from the menu 2. Make sure the switch is indented Test Plan 2: 1. Open about:styles 2. Click "commonForm" on TOC 3. Make sure the template of the commonForm is displayed 4. Click "Go back to top" 5. Click "sectionTitle" on TOC 6. Make sure section title components are displayed

It turns out that margin-bottom was missing from CommonFormSubSection, so I added it. Auditors: Test Plan: 1. Open about:styles 2. Click commonForm on TOC 3. Make sure that there is margin between CommonFormSubSection and CommonFormDropdown

luixxiul · 2017-04-21T18:31:37Z

Updated, and tests should be fine based on the results of my forked repo: https://travis-ci.org/luixxiul/browser-laptop/builds/224445488.

NejcZdovc · 2017-04-21T20:37:03Z

@luixxiul still some test are failing, looks like some selectors problem

luixxiul · 2017-04-22T02:59:42Z

It turned out the most errors are intermittent and not related with those PRs.

https://travis-ci.org/brave/browser-laptop/jobs/224455046#L2935
https://travis-ci.org/brave/browser-laptop/jobs/224455049#L2288
https://travis-ci.org/brave/browser-laptop/jobs/224455051#L3034

Just re-run the jobs on Travis manually to confirm the results

NejcZdovc

I went trough all test and they are not related to your PR anymore. Great job. LGTM

luixxiul added feature/about-pages refactoring labels Apr 11, 2017

luixxiul self-assigned this Apr 11, 2017

luixxiul requested review from cezaraugusto and bsclifton April 11, 2017 19:00

bsclifton added this to the 0.14.3 milestone Apr 11, 2017

luixxiul added the PR/needs-QA-attention ☕ label Apr 12, 2017

This was referenced Apr 12, 2017

Remove siteDetails.less #8235

Closed

Refactor syncTab.js with Aphrodite #8044

Merged

luixxiul commented Apr 17, 2017

View reviewed changes

NejcZdovc reviewed Apr 17, 2017

View reviewed changes

luixxiul commented Apr 17, 2017

View reviewed changes

luixxiul mentioned this pull request Apr 17, 2017

Add sectionTitle components to about:styles #8350

Closed

cezaraugusto reviewed Apr 18, 2017

View reviewed changes

NejcZdovc reviewed Apr 19, 2017

View reviewed changes

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

bsclifton approved these changes Apr 19, 2017

View reviewed changes

cezaraugusto approved these changes Apr 19, 2017

View reviewed changes

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

NejcZdovc suggested changes Apr 21, 2017

View reviewed changes

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

Suguru Hirahara added 3 commits April 22, 2017 03:27

Fix CommonFormSubSection

304f937

It turns out that margin-bottom was missing from CommonFormSubSection, so I added it. Auditors: Test Plan: 1. Open about:styles 2. Click commonForm on TOC 3. Make sure that there is margin between CommonFormSubSection and CommonFormDropdown

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

NejcZdovc approved these changes Apr 22, 2017

View reviewed changes

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

luixxiul merged commit 715a1b6 into brave:master Apr 22, 2017

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

This was referenced Apr 23, 2017

Add commonForm components to about:styles #8349

Closed

restructure paymentTab.js #7750

Closed

bsclifton mentioned this pull request Apr 23, 2017

Update header style on about:preferences pages #8165

Closed

luixxiul mentioned this pull request Apr 23, 2017

Remove brave.less with sectionTitle.js #8234

Closed

NejcZdovc mentioned this pull request Apr 23, 2017

Add configurable UI scaling #8419

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create sectionTitle.js #8237

Create sectionTitle.js #8237

luixxiul commented Apr 11, 2017 •

edited

Loading

luixxiul Apr 17, 2017

luixxiul Apr 17, 2017

luixxiul Apr 17, 2017

NejcZdovc Apr 17, 2017 •

edited

Loading

luixxiul Apr 17, 2017

NejcZdovc Apr 17, 2017 •

edited

Loading

luixxiul Apr 17, 2017 •

edited

Loading

luixxiul Apr 17, 2017

NejcZdovc Apr 17, 2017

NejcZdovc Apr 17, 2017

NejcZdovc Apr 17, 2017

luixxiul Apr 17, 2017

cezaraugusto Apr 17, 2017

luixxiul Apr 17, 2017

cezaraugusto left a comment

luixxiul commented Apr 19, 2017

NejcZdovc commented Apr 19, 2017 •

edited

Loading

luixxiul commented Apr 19, 2017 •

edited

Loading

NejcZdovc Apr 19, 2017

NejcZdovc Apr 21, 2017

cezaraugusto commented Apr 19, 2017

bsclifton commented Apr 19, 2017 •

edited

Loading

bsclifton left a comment

cezaraugusto left a comment

NejcZdovc left a comment

NejcZdovc commented Apr 21, 2017 •

edited

Loading

luixxiul commented Apr 21, 2017 •

edited

Loading

luixxiul commented Apr 21, 2017 •

edited

Loading

NejcZdovc commented Apr 21, 2017

luixxiul commented Apr 22, 2017 •

edited

Loading

NejcZdovc left a comment

Create sectionTitle.js #8237

Create sectionTitle.js #8237

Conversation

luixxiul commented Apr 11, 2017 • edited Loading

1. Created TitleWrapper, SectionTitle, and SubTitle

2. Refactored with those components:

3. Added siteDetailsPageContent to commonStyles.js and applied it to:

4. Removed styles of sectionTitle from:

Test Plans

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Apr 17, 2017 • edited Loading

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

cezaraugusto left a comment

Choose a reason for hiding this comment

luixxiul commented Apr 19, 2017

NejcZdovc commented Apr 19, 2017 • edited Loading

luixxiul commented Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezaraugusto commented Apr 19, 2017

bsclifton commented Apr 19, 2017 • edited Loading

bsclifton left a comment

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc commented Apr 21, 2017 • edited Loading

luixxiul commented Apr 21, 2017 • edited Loading

luixxiul commented Apr 21, 2017 • edited Loading

NejcZdovc commented Apr 21, 2017

luixxiul commented Apr 22, 2017 • edited Loading

NejcZdovc left a comment

Choose a reason for hiding this comment

luixxiul commented Apr 11, 2017 •

edited

Loading

1. Created `TitleWrapper`, `SectionTitle`, and `SubTitle`

3. Added `siteDetailsPageContent` to commonStyles.js and applied it to:

4. Removed styles of `sectionTitle` from:

NejcZdovc Apr 17, 2017 •

edited

Loading

NejcZdovc Apr 17, 2017 •

edited

Loading

luixxiul Apr 17, 2017 •

edited

Loading

NejcZdovc commented Apr 19, 2017 •

edited

Loading

luixxiul commented Apr 19, 2017 •

edited

Loading

bsclifton commented Apr 19, 2017 •

edited

Loading

NejcZdovc commented Apr 21, 2017 •

edited

Loading

luixxiul commented Apr 21, 2017 •

edited

Loading

luixxiul commented Apr 21, 2017 •

edited

Loading

luixxiul commented Apr 22, 2017 •

edited

Loading