Reorganize about pages and less files #10881

luixxiul · 2017-09-10T19:21:18Z

Requires #10951 for buttons on about:autofill.

Fixes #10711
Closes #8056
Closes #10700
Closes #10712
Closes #10880
Closes #11120

Add less/about/common.less to the about js files
Add globalStyles.spacing.aboutPageMargin
Add the MPL license header
Remove import from the less files
Remove button.less from common.less
Remove require of the less files imported in common.less to avoid duplicates
Move the height:100% property from common.less to preferences.less

Please have a look at #10880 why I thought this reorganization is important, thanks.

Test Plans

1st commit

Closes #10700
Closes #10880
Addresses #8056

Test Plan 1: npm run unittest

Test Plan 2:

Open about:about
Open all linked about pages
Make sure they are properly styled
Open https://webtorrent.io/free-torrents
Open a torrent link
Make sure the webtorrent viewer is properly styled

Test Plan 3:

Open https://expired.badssl.com (for certerror.js)
Open https://brave.rules (for errorPage.js)
Open about:preferences (for preferences.js)
Open http://downloadme.org (for safebrowsing.js)
Make sure the all error pages are rendered
Make sure all error pages do not have the black background color

Test Plan 4:

Open about:brave
Change the window size
Make sure the scroll bar does not appear

2nd commit

Fixes #10711
Closes #10712

Test Plan:

Open about:brave
Make sure the layout is not broken

3rd commit

Closes #8056

Test Plan:

Open about:about
Make sure the page is properly rendered

4th commit

Closes #11120

Test Plan:

Open about:about
Open about:adblock
Open about:autofill
Open about:brave
Open about:downloads
Open about:extensions
Make sure the section titles are styled in the same way

Submitter Checklist:

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).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

luixxiul · 2017-09-10T19:26:28Z

app/renderer/components/styles/global.js

+    panelPadding: '18px',
+
+    // about pages
+    aboutPageMargin: '20px',


Only this line is added. The others are just grouped below.

luixxiul · 2017-09-10T19:27:46Z

js/about/adblock.js


 const ipc = window.chrome.ipcRenderer

 // Stylesheets
-require('../../less/switchControls.less')


This is duping to https://github.com/brave/browser-laptop/pull/10881/files#diff-b79e96c438a5b97dc31f04f68d4817a7R7 on common.less

luixxiul · 2017-09-10T19:29:56Z

less/about/history.less

@@ -1,19 +1,13 @@
-@import "./siteDetails.less";


deprecated thanks to https://github.com/brave/browser-laptop/pull/10881/files#diff-237402632b025505a0517d846e9e32c1R34 on history.js

luixxiul · 2017-09-10T19:32:11Z

less/about/error.less

@@ -1,4 +1,8 @@
-@import "./common.less";


deprecated thanks to

https://github.com/brave/browser-laptop/pull/10881/files#diff-2369573669cb068e3f220a1c4ef7090cR13 on certerror.js

https://github.com/brave/browser-laptop/pull/10881/files#diff-103b59b981aa5f02b62b3f712ca69c19R12 on errorPage.js

luixxiul · 2017-09-10T19:33:36Z

less/about/common.less

@@ -13,10 +17,12 @@
  padding: 0;
 }

-html, body, #appContainer, #appContainer > div {
-  height: 100%;
-}


deprecated with https://github.com/brave/browser-laptop/pull/10881/files#diff-4b50ff14dadc9756719a577a72f520d6R7 on preferences.less. height:100% is required on preferences.js only.

luixxiul · 2017-09-10T19:34:49Z

app/renderer/about/bookmarks/bookmarks.js

@@ -21,6 +21,7 @@ const aboutActions = require('../../../../js/about/aboutActions')
 const windowActions = require('../../../../js/actions/windowActions')

 // Stylesheets
+require('../../../../less/about/siteDetails.less')


moved from https://github.com/brave/browser-laptop/pull/10881/files#diff-9161cb086bf3eb720aab15d7172f6e4fL1 on bookmarks.less

luixxiul · 2017-09-10T19:36:13Z

js/about/brave.js

 const commonStyles = require('../../app/renderer/components/styles/commonStyles')

 const {
  AboutPageSectionTitle,
  AboutPageSectionSubTitle
 } = require('../../app/renderer/components/common/sectionTitle')

-require('../../less/about/history.less')


Removed along with https://github.com/brave/browser-laptop/pull/10881/files#diff-6f6a63435cccded794332a0616b5f0e3L7, in favor of the converted styles below and siteDetails.less, which is more straightforward.

luixxiul · 2017-09-10T19:48:52Z

js/about/certerror.js

@@ -9,8 +9,8 @@ const messages = require('../constants/messages')
 const ipc = window.chrome.ipcRenderer
 const {isSourceAboutUrl, getTargetAboutUrl} = require('../lib/appUrlUtil')

-require('../../less/button.less')


Deprecated with common.less

luixxiul · 2017-09-10T19:49:55Z

js/about/errorPage.js

@@ -8,8 +8,8 @@ const BrowserButton = require('../../app/renderer/components/common/browserButto
 const appActions = require('../../js/actions/appActions')
 const tabActions = require('../../app/common/actions/tabActions')

-require('../../less/button.less')


Deprecated with https://github.com/brave/browser-laptop/pull/10881/files#diff-b79e96c438a5b97dc31f04f68d4817a7R6 on common.less

luixxiul · 2017-09-10T19:51:19Z

js/about/history.js

@@ -30,6 +30,8 @@ const {
 } = require('../../app/renderer/components/common/sectionTitle')

 // Stylesheets
+require('../../less/about/common.less')
+require('../../less/about/siteDetails.less')


Added as this is no longer imported on history.less

luixxiul · 2017-09-10T19:52:40Z

js/about/preferences.js

@@ -63,10 +63,9 @@ const ipc = window.chrome.ipcRenderer
 const hintCount = 3

 // Stylesheets
-require('../../less/switchControls.less')


Removed based on https://github.com/brave/browser-laptop/pull/10881/files#diff-b79e96c438a5b97dc31f04f68d4817a7R7 on common.less

luixxiul · 2017-09-10T19:52:55Z

js/about/preferences.js

 require('../../less/forms.less')
-require('../../less/button.less')


Ditto: on https://github.com/brave/browser-laptop/pull/10881/files#diff-b79e96c438a5b97dc31f04f68d4817a7R6 on common.less

luixxiul · 2017-09-10T19:53:33Z

js/about/safebrowsing.js

@@ -7,8 +7,8 @@ const BrowserButton = require('../../app/renderer/components/common/browserButto

 const {StyleSheet, css} = require('aphrodite/no-important')

-require('../../less/button.less')


Removed based on https://github.com/brave/browser-laptop/pull/10881/files#diff-b79e96c438a5b97dc31f04f68d4817a7R6 on common.less

luixxiul · 2017-09-10T19:54:32Z

less/about/autofill.less

@@ -1,4 +1,8 @@
-@import "./common.less";


Removed as it is required directly on autofill.js See: https://github.com/brave/browser-laptop/pull/10881/files#diff-f3189199f630a8432fae752aa453b2ccR15

luixxiul · 2017-09-10T19:55:40Z

less/about/bookmarks.less

@@ -1,4 +1,8 @@
-@import "./siteDetails.less";


Moved to https://github.com/brave/browser-laptop/pull/10881/files#diff-c74077737ab01e5191e480529919c489R24 on bookmarks.js

luixxiul · 2017-09-10T19:57:17Z

less/about/newtab.less

 @import "../animations.less";
-@import "./common.less";


Moved to

https://github.com/brave/browser-laptop/pull/10881/files#diff-491dd03a03308cec37fad2ad0dc7ba67R13 on newprivatetab.js

https://github.com/brave/browser-laptop/pull/10881/files#diff-dbb992f7cd2508b46d5cd98e609be5a5R36 on newtab.js

luixxiul · 2017-09-10T19:58:01Z

less/about/preferences.less

@@ -1,12 +1,11 @@
-@import "./common.less";


Moved to https://github.com/brave/browser-laptop/pull/10881/files#diff-e3eeb751016b2ce9f8278efce585a461R67 on preferences.js

luixxiul · 2017-09-10T20:01:58Z

less/about/siteDetails.less

-}
-
-body {
-  background: @veryLightGray;


Deprecated with https://github.com/brave/browser-laptop/pull/10881/files#diff-b79e96c438a5b97dc31f04f68d4817a7R22 on common.less.

As common.less is required on all of the about js files, it should not cause a regression.

luixxiul · 2017-09-10T20:03:23Z

less/webtorrent.less

-@import "./variables.less";
-@import "./button.less";
-@import "./about/siteDetails.less";
+@import "variables.less";


Deprecated on entry.js with

https://github.com/brave/browser-laptop/pull/10881/files#diff-9774044dc3cb63ab522b79039669a42aR17

https://github.com/brave/browser-laptop/pull/10881/files#diff-9774044dc3cb63ab522b79039669a42aR18

luixxiul

Left comments for reference, thanks.

luixxiul · 2017-09-11T06:17:17Z

Adding the impact/high label as there is a possibility of visual regressions.

luixxiul · 2017-09-11T09:33:37Z

js/about/about.js


 const {
  AboutPageSectionTitle,
  AboutPageSectionSubTitle
 } = require('../../app/renderer/components/common/sectionTitle')

-require('../../less/about/history.less')


Removed as siteDetails* classes are no longer applied.

codecov-io · 2017-10-25T17:32:22Z

Codecov Report

Merging #10881 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10881      +/-   ##
==========================================
+ Coverage   54.54%   54.55%   +<.01%     
==========================================
  Files         275      275              
  Lines       26144    26146       +2     
  Branches     4196     4196              
==========================================
+ Hits        14261    14263       +2     
  Misses      11883    11883

Flag	Coverage Δ
#unittest	`54.55% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
app/renderer/components/styles/global.js	`100% <ø> (ø)`	⬆️
js/about/newtab.js	`50% <100%> (+0.61%)`	⬆️
js/about/preferences.js	`45.23% <100%> (-0.12%)`	⬇️
js/about/newprivatetab.js	`80% <100%> (+0.83%)`	⬆️

luixxiul · 2017-11-21T08:28:19Z

@cezaraugusto this PR is blocked by #10951 (for buttons on about:autofill). I would like to see #10951 reviewed before this one 😄

Closes #10700 Closes #10880 Addresses #8056 - Add less/about/common.less to the about js files - Add globalStyles.spacing.aboutPageMargin - Add the MPL license header - Remove import from the less files - Remove button.less from common.less - Remove require of the less files imported in common.less to avoid duplicates - Move the height:100% property from common.less to preferences.less Auditors: Test Plan 1: npm run unittest Test Plan 2: 1. Open about:about 2. Open all linked about pages 3. Make sure they are properly styled 4. Open https://webtorrent.io/free-torrents 5. Open a torrent link 6. Make sure the webtorrent viewer is properly styled Test Plan 3: 1. Open https://expired.badssl.com/ 2. Open http://downloadme.org/ 3. Open http://brave.rules/ 4. Make sure all error pages do not have the black background color Test Plan 4: 1. Open about:brave 2. Change the window size 3. Make sure the scroll bar does not appear

Fixes #10711 Closes #10712 Auditors: Test Plan: 1. Open about:brave 2. Make sure the layout is not broken

Closes #8056 Auditors: Test Plan: 1. Open about:about 2. Make sure the page is properly rendered

Closes #11120 - about - adblock - autofill - brave - downloads - extensions - passwords Auditors: Test Plan: 1. Open about:about 2. Open about:adblock 3. Open about:autofill 4. Open about:brave 5. Open about:downloads 6. Open about:extensions 7. Make sure the section titles are styled in the same way

cezaraugusto · 2017-12-19T22:55:12Z

left a comment in #10951 the text inside buttons doesn't seem ok to me, mind checking there what's happening?

luixxiul · 2018-01-17T16:33:02Z

Let me close the PR until I'll find a better fix on #10951.

luixxiul mentioned this pull request Sep 10, 2017

Polish about:brave #10713

Closed

8 tasks

luixxiul self-assigned this Sep 10, 2017

luixxiul added feature/about-pages refactoring/aphrodite labels Sep 10, 2017

luixxiul commented Sep 10, 2017

View reviewed changes

luixxiul requested review from cezaraugusto and bsclifton September 10, 2017 20:06

luixxiul added the impact/high label Sep 11, 2017

luixxiul commented Sep 11, 2017

View reviewed changes

luixxiul mentioned this pull request Sep 11, 2017

Remove button.less requirement from about pages #10701

Closed

8 tasks

luixxiul mentioned this pull request Sep 24, 2017

Reorganize LESS files (import, require, etc) to normalize the design #10880

Closed

ghost removed the priority/P5 Cosmetic. Spelling, copy, layout. New features (which should also be part of an initiative). label Sep 26, 2017

jonathansampson mentioned this pull request Sep 28, 2017

Unnecessary Scrollbars on About Pages #10711

Closed

luixxiul added the PR/needs-rebase ۞ label Oct 3, 2017

bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017

luixxiul removed the PR/needs-rebase ۞ label Oct 25, 2017

bbondy modified the milestones: 0.20.x (Beta Channel), Backlog Oct 27, 2017

luixxiul added PR/pending-review QA/test-plan-specified labels Oct 30, 2017

luixxiul added the PR/needs-rebase ۞ label Nov 7, 2017

cezaraugusto removed the PR/blocked label Nov 21, 2017

luixxiul added PR/blocked and removed PR/needs-rebase ۞ labels Nov 21, 2017

bsclifton removed the impact/high label Nov 22, 2017

cezaraugusto added the PR/needs-rebase ۞ label Nov 24, 2017

Suguru Hirahara added 4 commits November 26, 2017 00:36

Polish about:brave

a6f62c8

Fixes #10711 Closes #10712 Auditors: Test Plan: 1. Open about:brave 2. Make sure the layout is not broken

Refactor about.js with Aphrodite

3047245

Closes #8056 Auditors: Test Plan: 1. Open about:about 2. Make sure the page is properly rendered

luixxiul removed the PR/needs-rebase ۞ label Nov 25, 2017

cezaraugusto added the PR/needs-rebase ۞ label Jan 13, 2018

luixxiul closed this Jan 17, 2018

luixxiul removed this from the Triage Backlog milestone Jan 17, 2018

bsclifton removed the PR/pending-review label Jan 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize about pages and less files #10881

Reorganize about pages and less files #10881

luixxiul commented Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017

luixxiul Sep 10, 2017

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul left a comment

luixxiul commented Sep 11, 2017

luixxiul Sep 11, 2017

codecov-io commented Oct 25, 2017 •

edited

Loading

luixxiul commented Nov 21, 2017

cezaraugusto commented Dec 19, 2017

luixxiul commented Jan 17, 2018

		require('../../less/forms.less')
		require('../../less/button.less')

		@@ -7,8 +7,8 @@ const BrowserButton = require('../../app/renderer/components/common/browserButto

		const {StyleSheet, css} = require('aphrodite/no-important')

		require('../../less/button.less')

Reorganize about pages and less files #10881

Reorganize about pages and less files #10881

Conversation

luixxiul commented Sep 10, 2017 • edited Loading

Test Plans

1st commit

2nd commit

3rd commit

4th commit

luixxiul Sep 10, 2017

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017

Choose a reason for hiding this comment

luixxiul Sep 10, 2017

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul left a comment

Choose a reason for hiding this comment

luixxiul commented Sep 11, 2017

luixxiul Sep 11, 2017

Choose a reason for hiding this comment

codecov-io commented Oct 25, 2017 • edited Loading

Codecov Report

luixxiul commented Nov 21, 2017

cezaraugusto commented Dec 19, 2017

luixxiul commented Jan 17, 2018

luixxiul commented Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

luixxiul Sep 10, 2017 •

edited

Loading

codecov-io commented Oct 25, 2017 •

edited

Loading