Add Auto-suggest sites option, to prefs and urlBar #6422

cezaraugusto · 2016-12-25T20:51:48Z

Note before merging: I cherry-picked PR #6101 from @jkup + polish from @luixxiul, which this PR is depedent. Will need rebase after those being approved.

PR adds a new "auto-suggest sites" option, and a "remote control" for that option on URL bar.

Screenshots:

Enabled publisher icon

Disabled publisher icon

New options on Preferences->Payments

Test plan

Automated tests:

npm run unittest -- --grep="PublisherToggle component"

Manual tests:

Case 1 (Publisher exists and is disabled)

Go to about:preferences -> Payments. Enable Payments and disable "auto-suggest sites" and "only show included sites" options;
Have at least two publishers on Payments. Ensure one of them is enabled;
Go to enabled publisher URL. Url bar should have an orange BitCoin button right after;
Clicking the button should turn it gray;
Back to Publishers panel, publisher should now be disabled;

Case 2 (Publisher exists and is enabled)

Same steps as the above;
Clicking the orange button should turn it gray;
Back to Publishers panel, publisher should now be enabled;

Case 3 (Publisher doesn't exist yet)

Go to a new publisher URL.
Spend enough time and visits to ensure Publisher can be on Payments list
On Publishers list, the newest publisher should be disabled;
Go back to newest publisher Url. Hit the BitCoin button. It should be orange;
Go to Publishers list again, publisher should be enabled;

Edge Case 1 (User has both "auto-suggest sites" and "only show included sites" options enabled)

Enable both "auto-suggest sites" and "only show incuded sites" options on Payments;
Go to a publisher url. BitCoin icon should not be visible;

Edge Case 2 (User has Payments disabled)

Disable "auto-suggest sites" and "only show included sites" options;
Disable Payments;
Go to a Publisher url, hit BitCoin icon;
Payments should be enabled

Merge notes

Must be merged AFTER #6101

bsclifton · 2016-12-26T18:44:23Z

@mrose17 I know you and @cezaraugusto have been working out the remaining work. As it stands now, what's left?

mrose17

LGTM

luixxiul · 2016-12-29T02:43:36Z

Pushed the commit 31206be to align the elements:

Also: #6285

luixxiul · 2016-12-29T02:45:54Z

less/about/preferences.less

    }
  }

  .paymentsMessage {
    background-color: @lightGray;
    border-radius: @borderRadiusUIbox;
-    margin: @walletBarMargin;
+    margin-top: @walletBarMargin;


This fixes the regression I introduced with the former commit of the PR.

Test Plan:

Disable payments

Make sure the sidebar is not wrapped

luixxiul · 2016-12-29T17:35:57Z

less/navigationBar.less

@@ -745,6 +743,12 @@
      min-height: @urlbarFormHeight;
      min-width: @urlbarFormHeight;
      -webkit-app-region: no-drag;


@cezaraugusto I added this here because I thought this is required for .addPublisherButtonContainer too. Is it OK?

ok, thanks!

bsclifton · 2017-01-05T08:53:27Z

@cezaraugusto I believe this is complete, right? Did you finish with tests?

@jkup had some great examples you can check out here:
#6460
#6529

bsclifton

I'm trying this out and just found a small issue... otherwise, it looks great

bsclifton · 2017-01-06T07:08:22Z

Here's the issue I found. If it's too large, we can create a new issue for it and resolve at a later date

Move session folder out of the way
Launch Brave and disable auto suggest sites (since it's enabled by default)
Visit "wikipedia.org" or "cnn.com" and let the site finish loading
Notice that the bitcoin button (to the right of the URL bar) is not visible
Refresh page
Notice that the bitcoin icon (to the right of the URL bar) is visible now

bsclifton · 2017-01-06T07:17:50Z

@mrose17 @bradleyrichter the PR still needs some work, but in the meantime, I have pushed a copy of Cezar's fork into the main brave/browser-laptop repo. You can access this with the branch auto-suggest-sites:
https://github.com/brave/browser-laptop/tree/auto-suggest-sites

cezaraugusto · 2017-01-06T15:12:12Z

Needs some rebase and tests, but works as-is. Thanks @bsclifton

luixxiul · 2017-01-08T05:12:22Z

@cezaraugusto I added tiny fixes.

bsclifton

As-is, we have a few problems ☹️

The hostPattern in navigationBar.js only allows for HTTPS, so this logic doesn't work for HTTP sites
The first page load doesn't show the icon. To demo, visit https://en.wikipedia.org/wiki/Main_Page. If the icon shows for you, pick another language on the left (which you haven't viewed before). When doing the first load, you'll notice the bitcoin icon does NOT show

cezaraugusto · 2017-01-09T15:12:26Z

@bsclifton, actually for hostPattern, it's just a pattern defined by ledger itself, so any location stored there follows the same rule, being HTTP/HTTPS. All I do is picking domain and adding https?://${getDomain} pattern, so protocol is ignored.

For first page load issue, I found that issue but was intermittent so thanks for describing STR. I did a push with a fix, lmk if that's was addressed.

@luixxiul rebased and amended with your previous one, lmk if it's good now, thanks!!

luixxiul · 2017-01-09T15:17:14Z

file changed count: 465?? @cezaraugusto

luixxiul · 2017-01-09T15:19:02Z

less/navigationBar.less

+      display: flex;
+      align-items: center;
+      height: 25px;
+      width: 25px;


Let's remove those added properties above in .addPublisherButtonContainer

cf: 2075547

luixxiul · 2017-01-09T15:29:04Z

@cezaraugusto It seems that you're including other files on your local machine

luixxiul · 2017-01-09T15:33:04Z

less/navigationBar.less

@@ -671,6 +671,7 @@
    content: ' ';
    position: absolute;
    background: #fff;
+    border: 1px solid @focusUrlbarOutline;


This needs removing too.

cf: https://github.com/cezaraugusto/browser-laptop/commit/7b4ab6ffc7a93385427a9424e1a1a74d6dd695f9

updated, thanks!

luixxiul · 2017-01-09T15:49:17Z

I checked LESS files were updated, thanks for updating :-)

bsclifton

OK- just gave this a go... awesome work 😄

Almost everything works as expected. I only noticed one problem. Basically, if I have these conditions:

fresh session (move the folder out of the way)
Auto suggest sites disabled

And I visit a site which I had not been to before (ex: brianbondy.com), then I am properly shown the bitcoin icon in the URL bar. However, if I click it and then go into preferences > Payments, it doesn't immediately get added to the list. This is because the list is considering the advanced settings.

Is there a way to force the site through? If so and it's an easy fix, let's do it 😄 otherwise let's create an issue and we can follow up on this soon!

(otherwise approved! again- great job 😄)

cezaraugusto · 2017-01-10T01:21:40Z

@bsclifton we could create an action to force the publisher to be on ledger payments and skip settings rules (min visit/min timestamp). Currently it will be enabled once you hit the bitCoin icon but will not be included unless advanced settings' rules are fulfilled.

Can't say however if we should delay this PR in favour of adding it, or it's ok to do that as a follow-up. @mrose17 what do you think?

luixxiul · 2017-01-10T03:12:05Z

js/about/preferences.js

+          this.props.ledgerData.get('created') && this.enabled
+          ? <Button
+            l10nId='advancedSettings'
+            className='advancedSettings whiteButton'


note for reference: https://github.com/brave/browser-laptop/pull/6388/files#diff-e3eeb751016b2ce9f8278efce585a461R1314 of #6388

bsclifton

@cezaraugusto I talked with @mrose17 and created #6592 as a follow up. Approved! 😄

luixxiul · 2017-01-11T02:49:39Z

@cezaraugusto: Will you please not drop my 7ccae16 of this PR as the original one ~~will be~~ was dropped instead of it? Thanks! #6101 (comment) /cc: @jkup

luixxiul · 2017-01-11T06:46:39Z

ok @cezaraugusto, because #6101 was merged into 0.13-branch, will you drop 3e791cb of this PR?

As I've asked you, please do not drop 7ccae16. Thanks :-)

cezaraugusto · 2017-01-13T07:18:16Z

PR updated with automated tests

luixxiul · 2017-01-13T07:25:52Z

@cezaraugusto please drop 3339539 which has been already merged in 0.13.1-branch

bsclifton · 2017-01-13T09:48:55Z

I rebased and did a force update and for some reason, GitHub closed this?!

bsclifton · 2017-01-13T10:05:46Z

ok I understand what I did wrong (this was completely my fault). I didn't set the tracking for your remote when I did the rebase ☹️ I didn't fetch before doing this either, so I don't have the commit in my reflog

Should be easy to fix though! @cezaraugusto, can you re-push your local branch to your fork? It should have the sha ecbadaa

luixxiul · 2017-01-13T12:30:45Z

@bsclifton should I too?

luixxiul · 2017-01-13T12:31:38Z

Otherwise please cherry-pick 7ccae16

Edit: Or I would by myself with another PR, which I think is better. Let me know :-)

cezaraugusto · 2017-01-13T17:50:42Z

@luixxiul follow-up here, rebased against 0.13.1-branch with your changes. Lmk if I missed something:

#6633

cezaraugusto self-assigned this Dec 25, 2016

cezaraugusto requested review from mrose17 and bsclifton December 25, 2016 20:51

cezaraugusto added feature/rewards QA/test-plan-specified feature/URLbar labels Dec 25, 2016

mrose17 approved these changes Dec 28, 2016

View reviewed changes

willy-b mentioned this pull request Dec 29, 2016

Add "Import recovery keys" button to Payments->Advanced->Recover #5809

Merged

4 tasks

luixxiul reviewed Dec 29, 2016

View reviewed changes

bsclifton added this to the 0.13.1 milestone Jan 5, 2017

bsclifton suggested changes Jan 6, 2017

View reviewed changes

luixxiul added the PR/work-in-progress ⚒ label Jan 6, 2017

jkup mentioned this pull request Jan 8, 2017

Split PaymentsTab into its own component #6529

Merged

4 tasks

bsclifton suggested changes Jan 9, 2017

View reviewed changes

luixxiul reviewed Jan 9, 2017

View reviewed changes

bsclifton suggested changes Jan 10, 2017

View reviewed changes

luixxiul mentioned this pull request Jan 10, 2017

restyle preferences payments #6101

Merged

4 tasks

luixxiul reviewed Jan 10, 2017

View reviewed changes

bsclifton mentioned this pull request Jan 10, 2017

Sites added manually (auto-suggest sites off) don't show if adv criteria not met #6592

Closed

bsclifton approved these changes Jan 10, 2017

View reviewed changes

bsclifton added PR/ready-for-merge and removed PR/work-in-progress ⚒ labels Jan 10, 2017

bsclifton changed the base branch from master to 0.13.1-branch January 13, 2017 09:29

bsclifton closed this Jan 13, 2017

luixxiul added needs-info Another team member needs information from the PR/issue opener. and removed PR/ready-for-merge labels Jan 13, 2017

cezaraugusto mentioned this pull request Jan 13, 2017

Add Auto-suggest sites option, to prefs and urlBar #6633

Merged

4 tasks

bsclifton removed this from the 0.13.1 milestone Jan 13, 2017

luixxiul added QA/no-qa-needed release-notes/exclude and removed needs-info Another team member needs information from the PR/issue opener. QA/test-plan-specified labels Jan 13, 2017

alexwykoff added QA/checked-macOS QA/checked-Linux QA/checked-Win32 QA/checked-Win64 labels Feb 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Auto-suggest sites option, to prefs and urlBar #6422

Add Auto-suggest sites option, to prefs and urlBar #6422

cezaraugusto commented Dec 25, 2016 •

edited

Loading

bsclifton commented Dec 26, 2016 •

edited

Loading

mrose17 left a comment

luixxiul commented Dec 29, 2016 •

edited

Loading

luixxiul Dec 29, 2016

luixxiul Dec 29, 2016 •

edited

Loading

cezaraugusto Dec 29, 2016

bsclifton commented Jan 5, 2017

bsclifton left a comment

bsclifton commented Jan 6, 2017 •

edited

Loading

bsclifton commented Jan 6, 2017

cezaraugusto commented Jan 6, 2017

luixxiul commented Jan 8, 2017

bsclifton left a comment

cezaraugusto commented Jan 9, 2017

luixxiul commented Jan 9, 2017

luixxiul Jan 9, 2017

luixxiul Jan 9, 2017

luixxiul commented Jan 9, 2017

luixxiul Jan 9, 2017

cezaraugusto Jan 9, 2017

luixxiul commented Jan 9, 2017

bsclifton left a comment

cezaraugusto commented Jan 10, 2017

luixxiul Jan 10, 2017

bsclifton left a comment

luixxiul commented Jan 11, 2017 •

edited

Loading

luixxiul commented Jan 11, 2017

cezaraugusto commented Jan 13, 2017

luixxiul commented Jan 13, 2017

bsclifton commented Jan 13, 2017

bsclifton commented Jan 13, 2017

luixxiul commented Jan 13, 2017

luixxiul commented Jan 13, 2017 •

edited

Loading

cezaraugusto commented Jan 13, 2017

Add Auto-suggest sites option, to prefs and urlBar #6422

Add Auto-suggest sites option, to prefs and urlBar #6422

Conversation

cezaraugusto commented Dec 25, 2016 • edited Loading

Test plan

Merge notes

bsclifton commented Dec 26, 2016 • edited Loading

mrose17 left a comment

Choose a reason for hiding this comment

luixxiul commented Dec 29, 2016 • edited Loading

luixxiul Dec 29, 2016

Choose a reason for hiding this comment

luixxiul Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

cezaraugusto Dec 29, 2016

Choose a reason for hiding this comment

bsclifton commented Jan 5, 2017

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Jan 6, 2017 • edited Loading

bsclifton commented Jan 6, 2017

cezaraugusto commented Jan 6, 2017

luixxiul commented Jan 8, 2017

bsclifton left a comment

Choose a reason for hiding this comment

cezaraugusto commented Jan 9, 2017

luixxiul commented Jan 9, 2017

luixxiul Jan 9, 2017

Choose a reason for hiding this comment

luixxiul Jan 9, 2017

Choose a reason for hiding this comment

luixxiul commented Jan 9, 2017

luixxiul Jan 9, 2017

Choose a reason for hiding this comment

cezaraugusto Jan 9, 2017

Choose a reason for hiding this comment

luixxiul commented Jan 9, 2017

bsclifton left a comment

Choose a reason for hiding this comment

cezaraugusto commented Jan 10, 2017

luixxiul Jan 10, 2017

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

luixxiul commented Jan 11, 2017 • edited Loading

luixxiul commented Jan 11, 2017

cezaraugusto commented Jan 13, 2017

luixxiul commented Jan 13, 2017

bsclifton commented Jan 13, 2017

bsclifton commented Jan 13, 2017

luixxiul commented Jan 13, 2017

luixxiul commented Jan 13, 2017 • edited Loading

cezaraugusto commented Jan 13, 2017

cezaraugusto commented Dec 25, 2016 •

edited

Loading

bsclifton commented Dec 26, 2016 •

edited

Loading

luixxiul commented Dec 29, 2016 •

edited

Loading

luixxiul Dec 29, 2016 •

edited

Loading

bsclifton commented Jan 6, 2017 •

edited

Loading

luixxiul commented Jan 11, 2017 •

edited

Loading

luixxiul commented Jan 13, 2017 •

edited

Loading