Add compact bravery panel with Aphrodite #8990

luixxiul · 2017-05-21T17:09:09Z

Closes #8954: Add compact bravery panel with Aphrodite
Closes #8913: Refactor braveryPanel with Aphrodite
Closes #5726: The bravery panel should be much wider
Fixes #8953: Align 'Reload' text and icon
Fixes #8952: Set opacity to FormDropdown's wrappers and their titles
Fixes #8949: Rename 'general.blocked-count-badge' to 'shields.blocked-count-badge'
Addresses #7989: Refactor forms.less with Aphrodite
Addresses #7321: Refactor switchControl

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

Test plan 1:

Run tests locally; npm run test -- --grep='Bravery Panel'
Open about:preferences#shields
Enable the compact panel option
Run manual tests described here: https://github.com/brave/browser-laptop/wiki/Manual-Tests#bravery-settings

Test Plan 2:

Open about:preferences#shields
Make sure 'Display block count badge on shields button' works

Test Plan 3:

Open about:styles
Click "Dropdowns"
Make sure BraveryPanelDropdown is displayed

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

Closes #8954 Fixes #8953: Align 'Reload' text and icon Fixes #8952: Set opacity to FormDropdown's wrappers and their titles Fixes #8949: Rename 'general.blocked-count-badge' to 'shields.blocked-count-badge' Closes #5726: The bravery panel should be much wider Addresses #8913: Refactor braveryPanel.js with Aphrodite Addresses #7989: Refactor forms.less with Aphrodite ---- Replace display:flex with display:grid (grid makes it easier than flex to place elements) Reorganize styles - braveryPanelHeader - braveryPanelStats - braveryPanelBody - controlWrapper Based on Cezar's feedback - Replace '.braveryPanelBody li' with braveryPanelBodyList - Move data-l10n-id - Move 'large' inside SwitchControl - Create compactBraveryHeader and defaultBraveryPanelHeader Also: - BEM optimization - Introduce 'gridStyles' - Remove redundant divs and div wrappers from counters - Update tests Test plan 1. Run tests locally; npm run test -- --grep='Bravery Panel' 2. Open about:preferences#shields 3. Enable the compact panel option 4. Run manual tests described here: https://github.com/brave/browser-laptop/wiki/Manual-Tests#bravery-settings Test Plan 2: 1. Open about:preferences#shields 2. Make sure 'Display block count badge on shields button' works

Closes #8913 Also: - Add no-important to Aphrodite - Add shields settings to state.md - Remove .trackersBlockedStat which has not been used - Create BraveryPanelDropdown - Add BraveryPanelDropdown to about:styles Addresses #7321: Refactor switchControl - customWrapper - customTopText - customInfoButton Remove reduntant classNames by replacing cx with css (Note: some of the classNames were intentionally unchanged for readability) Test Plan 1: 1. Run tests locally; npm run test -- --grep='Bravery Panel' 2. Open about:preferences#shields 3. Enable the compact panel option 4. Run manual tests described here: https://github.com/brave/browser-laptop/wiki/Manual-Tests#bravery-settings Test Plan 2: 1. Open about:styles 2. Click "Dropdowns" 3. Make sure BraveryPanelDropdown is displayed

Related with #8986 - Reorganize const

luixxiul · 2017-05-21T17:13:56Z

On about:preferences#shields:

If the option is off:

If the option is on:

Updated to this one with #9078.

Jacalz · 2017-05-21T18:25:02Z

I must say that this looks amazing and I will most certainly use it 👍

luixxiul · 2017-05-21T19:07:35Z

From:

To:

cezaraugusto · 2017-05-22T04:07:10Z

app/renderer/components/common/dropdown.js

@@ -37,6 +38,12 @@ class SettingDropdown extends ImmutableComponent {
  }
 }

+class BraveryPanelDropdown extends ImmutableComponent {
+  render () {
+    return <FormDropdown data-isBraveryPanel='true' {...this.props} />


I think if you just let data-isBraveryPanel works. Above you're passing true as a string, which would evaluate prop to true anyway, so would any other string. For example, data-isBraveryPanel='brave' would be true as well.

You can double-check that if you pass data-isBraveryPanel={true}which will evaluate to boolean. Standardjs will warn you saying that it is not necessary:

Value must be omitted for boolean attributes

I would just <FormDropdown data-isBraveryPanel {...this.props} />.

Fixed with 5670822

cezaraugusto · 2017-05-22T04:31:19Z

app/renderer/components/main/braveryPanel.js

+              [css(styles.braveryPanel__stats__item_count_clickable)]: !!adsBlockedStat,
+              [css(styles.braveryPanel__stats__item_count_disabled)]: !shieldsUp || adControl === 'allowAdsAndTracking',
+              [css(gridStyles.row1col1)]: !compactBraveryPanel,
+              [css(gridStyles.row1col1)]: compactBraveryPanel,


if gridStyles.row1col1 appear in both conditions then you could only [css(gridStyles.row1col1)]: true

sure, I just missed that.

cezaraugusto · 2017-05-22T04:38:39Z

app/renderer/components/main/braveryPanel.js

+              [css(gridStyles.row1col1)]: compactBraveryPanel,
+              [css(styles.braveryPanel__stats__item_count_adsBlockedStat)]: true,
+              [css(styles.braveryPanel__stats__item, styles.braveryPanel__stats__item_count)]: !compactBraveryPanel,
+              [css(styles.braveryPanel_compact__stats__item_count)]: compactBraveryPanel


since all styles use Aphrodite we could make use of its recommended syntax as well:

className={css( !!adsBlockedStat && styles.braveryPanel__stats__item_count_clickable, (!shieldsUp || adControl === 'allowAdsAndTracking') && styles.braveryPanel__stats__item_count_disabled, gridStyles.row1col1, styles.braveryPanel__stats__item_count_adsBlockedStat, !compactBraveryPanel && styles.braveryPanel__stats__item, !compactBraveryPanel && styles.braveryPanel__stats__item_count, compactBraveryPanel && styles.braveryPanel_compact__stats__item_count )}

it may seem weird at first but it looks for the first assumption and only execute the latest if condition evaluates to true. condition && style would write nothing if condition is false.

Same tip applies for next cases. Let me know if you need a hand rewriting it

that's an example however. I would make (!shieldsUp || adControl === 'allowAdsAndTracking') a variable for better readability and put styles that don't need conditionals (such as gridStyles.row1col1) at first for the same reason.

I have already tried ^^ once but don't you think it is hard to read? (well it is, at least for me..)

In this case I would stick to cx for readability of classNames in BEM, still I'm fine with your suggestion too.

that's an example however.

I'm going to try that and compare it with the current version.

Fixed with 6b041ce

cezaraugusto · 2017-05-22T04:43:18Z

app/renderer/components/main/braveryPanel.js

+  whiteSpace: 'nowrap',
+  overflow: 'hidden'
+}
+const editGlobalMarginBottom = '.25rem'


I don't mind having relative units but curious why we have it?

Since I set 1rem to padding of braveryPanel_compact__body and I wanted to set .25rem as 25% of that padding. We know 1rem=16px, so it should be clear that .25rem=16px*25% and .75rem=16px*75% without doing math (no matter how easy it is in this case).

Using px sometimes makes it hard to know how and why this value has been set so. that's why I prefer relative units for elements which I added by myself. I avoided converting the units of other elements which already have existed because we can trust the value.

cezaraugusto · 2017-05-22T04:43:46Z

app/renderer/components/main/braveryPanel.js

+  },
+  row8col1: {
+    gridRow: 8,
+    gridColumn: 1


grid ftw ++

cezaraugusto · 2017-05-22T04:45:13Z

app/renderer/components/main/braveryPanel.js

+    right: '20px',
+    userSelect: 'none',
+    cursor: 'default',
+    color: '#3B3B3B',


can we move it to global.js and so other colors? you can copy browserButton way and create an object like:

braveryPanel: { color: '#3B3B3B' }

Done with a237c9d

cezaraugusto · 2017-05-22T04:51:19Z

comments aside, changes look great and new look will please a lot of users. Can't wait to see this landing

luixxiul

Addressed feedbacks.

cezaraugusto

this looks incredible ++

I would leave a comment but I think we have a lot of improvements here so I created #9016 to keep track of that. Will label as ready-for-merge feel free to merge once squashed.

- Replace cx with css - Fix data-isBraveryPanel - Move color properties to global.js Also: - Add braveryPanel__stats__item_label_clickable - Add braveryPanel__stats__item_label_disabled - Remove globalStyles.color.statsGray It is no longer used: https://github.com/brave/browser-laptop/blob/b831100570616921b388ab4f79fc60b751616642/less/forms.less#L89

luixxiul · 2017-05-23T19:13:23Z

@cezaraugusto thanks! Let's discuss the follow ups there.

bsclifton · 2017-05-23T21:09:04Z

Great job with this, @luixxiul! 😻

Suguru Hirahara added 3 commits May 22, 2017 01:59

Fix automated tests

0321b8f

Related with #8986 - Reorganize const

luixxiul added feature/shields refactoring/aphrodite labels May 21, 2017

luixxiul added this to the 0.16.100 milestone May 21, 2017

luixxiul self-assigned this May 21, 2017

luixxiul added the PR/needs-QA-attention ☕ label May 21, 2017

cezaraugusto reviewed May 22, 2017

View reviewed changes

cezaraugusto requested a review from diracdeltas May 22, 2017 04:51

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

luixxiul commented May 23, 2017

View reviewed changes

cezaraugusto mentioned this pull request May 23, 2017

Compact Bravery panel tracking issue #9016

Closed

5 tasks

cezaraugusto approved these changes May 23, 2017

View reviewed changes

cezaraugusto added PR/ready-for-merge and removed PR/needs-QA-attention ☕ labels May 23, 2017

luixxiul merged commit 8d01a21 into brave:master May 23, 2017

luixxiul removed the PR/ready-for-merge label May 23, 2017

This was referenced May 23, 2017

Add compact bravery panel with Aphrodite #8954

Closed

Refactor braveryPanel with Aphrodite #8913

Closed

Set opacity to FormDropdown when the shield is down #8952

Closed

luixxiul mentioned this pull request May 28, 2017

UI tweak for storage blocking in Bravery Panel and prefs #2398

Closed

luixxiul removed the request for review from diracdeltas June 2, 2017 04:34

luixxiul mentioned this pull request Jun 7, 2017

Polish compact Bravery panel #9078

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compact bravery panel with Aphrodite #8990

Add compact bravery panel with Aphrodite #8990

luixxiul commented May 21, 2017 •

edited

Loading

luixxiul commented May 21, 2017 •

edited

Loading

Jacalz commented May 21, 2017

luixxiul commented May 21, 2017

cezaraugusto May 22, 2017

luixxiul May 22, 2017

cezaraugusto May 22, 2017

luixxiul May 22, 2017

cezaraugusto May 22, 2017

cezaraugusto May 22, 2017

luixxiul May 22, 2017

luixxiul May 22, 2017

luixxiul May 22, 2017

cezaraugusto May 22, 2017

luixxiul May 22, 2017

cezaraugusto May 22, 2017

cezaraugusto May 22, 2017

luixxiul May 22, 2017

cezaraugusto commented May 22, 2017

luixxiul left a comment

cezaraugusto left a comment

luixxiul commented May 23, 2017

bsclifton commented May 23, 2017

Add compact bravery panel with Aphrodite #8990

Add compact bravery panel with Aphrodite #8990

Conversation

luixxiul commented May 21, 2017 • edited Loading

luixxiul commented May 21, 2017 • edited Loading

Jacalz commented May 21, 2017

luixxiul commented May 21, 2017

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

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 commented May 22, 2017

luixxiul left a comment

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

luixxiul commented May 23, 2017

bsclifton commented May 23, 2017

luixxiul commented May 21, 2017 •

edited

Loading

luixxiul commented May 21, 2017 •

edited

Loading