Polish compact Bravery panel #9078

luixxiul · 2017-05-26T18:04:05Z

Closes #9077
Addresses #9016

Requires #9043 (Remove adblock.less & Rename custom classNames on switchControl.js)

Test Plan:

Visit twitter.com
Make sure only counts of blocked resources are clickable
Make sure the panel is displayed as follows:

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

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-05-27T01:58:14Z

Moving the milestone as #8954 was merged for 0.16.1

luixxiul · 2017-05-31T00:37:50Z

Fixed class names with 6163ea4, which was squashed.

luixxiul · 2017-05-31T10:13:54Z

app/renderer/components/main/braveryPanel.js

 </section>
 }

 get defaultBraveryPanelHeader () {
 const shieldsUp = this.props.braverySettings.shieldsUp
 return <section className={css(styles.braveryPanel__header)}>
- <div className={css(styles.braveryPanel__header_left)}>
+ <div className={css(styles.braveryPanel__header__left)}>
 <div data-l10n-id='braveryPanelTitle' />
 <div title={this.displayHost} className={css(styles.braveryPanel__header__displayHost)}>{this.displayHost}</div>


This should be braveryPanel__header__left__displayHost. I'm fixing it.

Fixed with 39378e5

cezaraugusto · 2017-06-02T02:19:41Z

sorry for delaying review. could you pls rebase?

cezaraugusto · 2017-06-05T02:59:34Z

app/renderer/components/main/braveryPanel.js

@@ -8,6 +8,7 @@ const Immutable = require('immutable')
 // Components
 const ImmutableComponent = require('../immutableComponent')
 const Dialog = require('../common/dialog')
+const Button = require('../common/button')


any reason why prefer Button over BrowserButton?

no, I have just forgot about that.

I'll replace it!

Fixed with c949c8b

Closes #9077 Addresses #9016

Also: - Add padding around braveryPanel_compact__body__ul Auditors: Test Plan: 1. Visit twitter.com 2. Make sure only counts of blocked resources are clickable

cezaraugusto · 2017-06-05T21:12:30Z

everything works as expected except for a test which is perma failing for me:

"Bravery Panel Adblock stats without iframe tests blocks websocket tracking"
Ref: https://travis-ci.org/brave/browser-laptop/jobs/239658853#L3143.

Can you check what is missing there? At least rebase against master and see if fail persists since it works on the latest I tested (sha cdaee37). Ref: https://travis-ci.org/brave/browser-laptop/jobs/239658304.

cezaraugusto · 2017-06-06T22:51:36Z

app/renderer/components/main/braveryPanel.js

@@ -262,55 +271,63 @@ class BraveryPanel extends ImmutableComponent {
 compactBraveryPanel && styles.braveryPanel_compact__stats
 )}>
 <div data-test-id='adsBlockedStat'
- onClick={this.onToggleAdsAndTracking}


now that onClick handler is set for <span> the data-test-id should be moved over there as well. Otherwise, some tests that depend on it will perma fail because it is clicking an element that has no listener for changes.

cezaraugusto · 2017-06-06T22:52:20Z

app/renderer/components/main/braveryPanel.js

 (!shieldsUp || adControl === 'allowAdsAndTracking') && styles.braveryPanel__stats__item_count_disabled,
 gridStyles.row1col1,
 !compactBraveryPanel && styles.braveryPanel__stats__item,
 !compactBraveryPanel && styles.braveryPanel__stats__item_count,
 compactBraveryPanel && styles.braveryPanel_compact__stats__item_count
- )}>{adsBlockedStat}</div>
+ )}>
+ <span onClick={this.onToggleAdsAndTracking}


based on above, test id should belong to here, i.e. <span data-test-id='adsBlockedStat' onClick={this.onToggleAdsAndTracking} ...

Auditors: Test Plan: make sure the intermittent failures on Travis are fixed

cezaraugusto · 2017-06-07T02:47:59Z

not a blocker but let's track this one for the next polish, overflow for tracker list should be auto here to avoid empty scrollbar when there's no overflow:

cezaraugusto

UI is great, test plan works, no regression for automated tests, great work ++

luixxiul · 2017-06-07T02:50:27Z

@cezaraugusto nice find, I'll log that.

cezaraugusto · 2017-06-07T03:06:01Z

I guess there are some missing parts prior to this PR that weren't tagged for 0.17.x which lead to bad UI after pulling these. To avoid regressions I'm pushing back PR changes to 0.18.x

luixxiul · 2017-06-07T03:29:33Z

which isn't this one? #8954 that has been tagged with 0.17

luixxiul · 2017-06-07T06:35:03Z

It depends on https://github.com/brave/browser-laptop/pull/9043/files#diff-f9e9c72cdac7b145336979a3e76f20ebR195

Cherry-picking #9043 should fix the issue.

bsclifton · 2017-06-07T06:43:39Z

@luixxiul this worked great! I pulled this and the other polish commit into 0.17.x

luixxiul · 2017-06-07T06:44:00Z

thanks!

cezaraugusto · 2017-06-07T13:39:15Z

great ++

luixxiul added feature/shields polish Nice to have — usually related to front-end/visual tasks. PR/needs-QA-attention ☕ labels May 26, 2017

luixxiul added this to the 0.16.200 milestone May 26, 2017

luixxiul self-assigned this May 26, 2017

luixxiul modified the milestones: 0.16.200, 0.16.100 (Frozen, only critical adds from here) May 27, 2017

luixxiul requested a review from cezaraugusto May 27, 2017 11:40

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

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

luixxiul commented May 31, 2017

View reviewed changes

cezaraugusto added the PR/needs-rebase ۞ label Jun 2, 2017

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

luixxiul requested review from cezaraugusto and removed request for cezaraugusto June 3, 2017 07:07

luixxiul removed PR/needs-rebase ۞ PR/work-in-progress ⚒ labels Jun 3, 2017

cezaraugusto reviewed Jun 5, 2017

View reviewed changes

luixxiul added the PR/work-in-progress ⚒ label Jun 5, 2017

Suguru Hirahara added 2 commits June 6, 2017 02:14

Polish compact Bravery panel

89759f4

Closes #9077 Addresses #9016

Make only counts clickable

43a50d5

Also: - Add padding around braveryPanel_compact__body__ul Auditors: Test Plan: 1. Visit twitter.com 2. Make sure only counts of blocked resources are clickable

luixxiul requested a review from cezaraugusto June 5, 2017 17:15

luixxiul removed the PR/work-in-progress ⚒ label Jun 5, 2017

cezaraugusto reviewed Jun 6, 2017

View reviewed changes

Fix intermittent automated test failures

016084c

Auditors: Test Plan: make sure the intermittent failures on Travis are fixed

cezaraugusto approved these changes Jun 7, 2017

View reviewed changes

cezaraugusto merged commit b58e813 into brave:master Jun 7, 2017

luixxiul deleted the compactBraveryPanel-v3 branch June 7, 2017 02:50

luixxiul mentioned this pull request Jun 7, 2017

Polish compact bravery panel #9077

Closed

luixxiul mentioned this pull request Jun 7, 2017

Hide empty scrollbar on bravery panel #9296

Closed

cezaraugusto modified the milestones: 0.18.x, 0.17.x (Frozen, only critical adds from here) Jun 7, 2017

luixxiul modified the milestones: 0.17.x (Frozen, only critical adds from here), 0.18.x Jun 7, 2017

bsclifton modified the milestones: 0.18.x, 0.17.x (Frozen, only critical adds from here) Jun 7, 2017

luixxiul mentioned this pull request Jun 7, 2017

Remove adblock.less & Rename custom classNames on switchControl.js #9043

Merged

7 tasks

bsclifton pushed a commit that referenced this pull request Jun 7, 2017

Polish compact Bravery panel (#9078)

b84aaf0

luixxiul modified the milestones: 0.17.x (Frozen, only critical adds from here), 0.18.x Jun 7, 2017

This was referenced Jun 8, 2017

Create PULL_REQUEST_TEMPLATE.md #9337

Closed

Add compact bravery panel with Aphrodite #8990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish compact Bravery panel #9078

Polish compact Bravery panel #9078

luixxiul commented May 26, 2017 •

edited

Loading

luixxiul commented May 27, 2017

luixxiul commented May 31, 2017

luixxiul May 31, 2017

luixxiul May 31, 2017

cezaraugusto commented Jun 2, 2017

cezaraugusto Jun 5, 2017

luixxiul Jun 5, 2017

luixxiul Jun 5, 2017

luixxiul Jun 5, 2017

cezaraugusto commented Jun 5, 2017

cezaraugusto Jun 6, 2017

cezaraugusto Jun 6, 2017

cezaraugusto commented Jun 7, 2017

cezaraugusto left a comment

luixxiul commented Jun 7, 2017

cezaraugusto commented Jun 7, 2017

luixxiul commented Jun 7, 2017 •

edited

Loading

luixxiul commented Jun 7, 2017 •

edited

Loading

bsclifton commented Jun 7, 2017

luixxiul commented Jun 7, 2017

cezaraugusto commented Jun 7, 2017

Polish compact Bravery panel #9078

Polish compact Bravery panel #9078

Conversation

luixxiul commented May 26, 2017 • edited Loading

luixxiul commented May 27, 2017

luixxiul commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezaraugusto commented Jun 2, 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

cezaraugusto commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezaraugusto commented Jun 7, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

luixxiul commented Jun 7, 2017

cezaraugusto commented Jun 7, 2017

luixxiul commented Jun 7, 2017 • edited Loading

luixxiul commented Jun 7, 2017 • edited Loading

bsclifton commented Jun 7, 2017

luixxiul commented Jun 7, 2017

cezaraugusto commented Jun 7, 2017

luixxiul commented May 26, 2017 •

edited

Loading

luixxiul commented Jun 7, 2017 •

edited

Loading

luixxiul commented Jun 7, 2017 •

edited

Loading