restyle preferences payments #6101

jkup · 2016-12-08T22:23:52Z

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

Fix #5494

Auditors @bsclifton @bradleyrichter

Here is a screenshot of this branch:

Spec image:

jkup · 2016-12-09T05:09:55Z

@bsclifton there is a seemingly related test failing on this build but it appears to be failing on master too

synopsis can disable site:
Error: Promise was rejected with the following reason: TypeError: Cannot read property 'ledgerPayments' of undefined

Is this a known failure?

bsclifton · 2016-12-09T07:28:02Z

Just checked it out...

The verified icon doesn't show properly
Clicking remove does kill the entry. I can then go to shields and undo the removal... but the data appears to be lost for that. Is this intentional? I know this PR is doing the same thing as when you right click and pick "Do not show this entry". Maybe @mrose17 or @bradleyrichter can confirm this is correct
The table looks great! 😄 love seeing it match the others
~~The removal of the 60% style on sortableTable breaks the history page. It may have been sloppy of me to introduce that (instead of styling the columns individually).~~ Fixed! 😄

The other sortableTables look OK

Preferences > Search
Bookmarks manager
about:brave

The torrent viewer and torrent file list pages use the sortable tables too, but I'm not familiar with how to pull them up. @alexwykoff, @srirambv, or @luixxiul - can one of you help me with that?

luixxiul · 2016-12-09T07:35:32Z

less/about/preferences.less


-    td {
-      position: relative;


removing this breaks the position of the verified icon.

bsclifton · 2016-12-09T08:24:23Z

@jkup regarding the test- I tried it out... I think the test is written correct; it probably broke though when the default time for a visit was bumped up. We can likely increase that timeout to fix the issue. Since it's failing on master, I made an issue to track it:
#6110

jkup · 2016-12-13T22:16:04Z

@bsclifton is this one ready to merge?

bsclifton · 2016-12-14T15:45:51Z

@jkup yup! looks good to me 😄

luixxiul · 2016-12-14T15:58:18Z

Was the position of verified icon confirmed?

bsclifton · 2016-12-14T17:12:35Z

@luixxiul thanks for the reminder! No- it still does not look correct

@jkup can you fix it up please? 😄

jkup · 2016-12-14T20:04:32Z

@luixxiul @bsclifton doh! thanks for reminding me! It actually looks like we can add position:relative to all sortableTable td's and they all look great! Updated the commit.

bsclifton · 2016-12-15T19:26:17Z

@jkup can you rebase? thanks 😄

jkup · 2016-12-15T20:31:36Z

Done. I ran into a lot of merge conflicts but I checked all our sortable tables and they appear to look right!

luixxiul · 2016-12-16T00:42:09Z

less/about/preferences.less

+  margin: 10px 75px;
+}
+
+.modal .dialog.paymentHistory .sectionTitle {


I think you can remove this.

luixxiul · 2016-12-16T00:42:19Z

less/about/preferences.less

+    text-align: left;
+  }
+
+  #paymentHistory {


jkup · 2016-12-16T01:16:41Z

Thanks @luixxiul! Updated.

luixxiul · 2016-12-17T05:34:08Z

This change causes some regressions on about:preferences#payments. I will make it more specific, adding notes in preferences.less

jkup · 2016-12-17T05:50:23Z

Yeah I had put this PR up before you made all those design tweaks to prefs#payments. If you could let me know anything specific that would be great and I'll take care of it.

luixxiul · 2016-12-17T07:06:48Z

@jkup thanks, I'll add a commit soon. Please don't squash it and let @bsclifton review it ;-)

bsclifton · 2017-01-09T19:19:44Z

@luixxiul there is a CSS/LESS conflict here; can you please resolve this (either in GitHub's UI or by checking out / rebasing via command line). Thanks 😄

luixxiul · 2017-01-10T00:50:37Z

Otherwise I'm wondering if this PR should be closed as I think this has been superseded with #6422, where those two commits were cherry-picked. Also the PR includes fixes which are not added to this PR.

wdyt @jkup @bsclifton ?

bsclifton · 2017-01-10T05:21:08Z

@luixxiul let's keep 'em separate for now. When it's "go time", we'll want to merge this one first. Each subsequent PR will then require a rebase anyways, where we can remove the cherry picked commits

luixxiul · 2017-01-10T05:28:15Z

Ah right, we can remove the cherry-picked commits and I've totally forgotten about that! :D

bsclifton · 2017-01-10T21:37:31Z

This one is failing tests: cc: @jkup
npm run test -- --grep="synopsis can disable site"

I think it's looking in the wrong place. Here's a dump of the appState:
https://gist.github.com/bsclifton/4ec2c3bf55b13ef4ac8cf4564dcf8380

@alexwykoff

fix #2957 note that this partially un-fixes #1928. Auditors: @alexwykoff Test Plan: 1. go to developer.apple.com 2. click on 'Account' in the top right 3. enter your apple username and password 4. the login page should redirect to https://developer.apple.com/ 5. after the redirect, you should still see a password manager dialog from Brave that says 'Would you like Brave to remember the password for [username] on https://idmsa.apple.com?'

jkup · 2017-01-11T02:33:02Z

@bsclifton test is fixed!

@luixxiul let's try not to add commits to existing PRs. Should your CSS commit be rebased into mine? If so, can you rebase the now 3 commits into one? If not, maybe pull yours out into a separate ticket so I can rebase my two?

luixxiul · 2017-01-11T02:44:19Z

@jkup: please drop my 886cf71 of this PR, as it was cherry-picked in #6422 and I asked @cezaraugusto to use it. / cc: @cezaraugusto

bsclifton · 2017-01-11T06:42:09Z

Last travis-ci run shows 2 unrelated about:bookmark errors; I re-ran those locally and it's working as expected 😄 Merging...

jkup added design A design change, especially one which needs input from the design team. QA/test-plan-specified labels Dec 8, 2016

jkup added this to the 0.13.1 milestone Dec 8, 2016

luixxiul added the feature/rewards label Dec 9, 2016

luixxiul reviewed Dec 9, 2016

View reviewed changes

bradleyrichter mentioned this pull request Dec 10, 2016

Add "hide sites if" options to Advanced Options Dialog #4681

Closed

jkup force-pushed the prefs-payments branch from 15b3da9 to 48f3760 Compare December 14, 2016 05:32

bsclifton self-assigned this Dec 14, 2016

bsclifton added the ready-for-merge label Dec 14, 2016

bsclifton removed the ready-for-merge label Dec 14, 2016

jkup force-pushed the prefs-payments branch from 48f3760 to 37b1e96 Compare December 14, 2016 20:03

jkup force-pushed the prefs-payments branch from 37b1e96 to d63e9be Compare December 15, 2016 20:30

luixxiul reviewed Dec 16, 2016

View reviewed changes

less/about/preferences.less

margin: 10px 75px;

}

.modal .dialog.paymentHistory .sectionTitle {

Copy link

Contributor

luixxiul Dec 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this.

luixxiul reviewed Dec 16, 2016

View reviewed changes

less/about/preferences.less

text-align: left;

}

#paymentHistory {

Copy link

Contributor

luixxiul Dec 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

jkup force-pushed the prefs-payments branch from d63e9be to 5e6bc64 Compare December 16, 2016 01:16

luixxiul force-pushed the prefs-payments branch from 5e6bc64 to a9977b7 Compare December 17, 2016 07:21

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

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

bsclifton force-pushed the prefs-payments branch from 888f965 to 8f7f155 Compare January 10, 2017 20:18

jkup force-pushed the prefs-payments branch from 8f7f155 to ac78127 Compare January 11, 2017 02:31

restyle preferences payments

0f0fc38

jkup force-pushed the prefs-payments branch from ac78127 to 0f0fc38 Compare January 11, 2017 03:25

bsclifton changed the base branch from master to 0.13.1-branch January 11, 2017 06:41

bsclifton merged commit 2161945 into 0.13.1-branch Jan 11, 2017

bsclifton deleted the prefs-payments branch January 11, 2017 06:42

luixxiul removed the PR/ready-for-merge label Jan 11, 2017

cezaraugusto mentioned this pull request Jan 13, 2017

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

Merged

4 tasks

luixxiul removed the QA/test-plan-specified label Jan 18, 2017

This was referenced Feb 3, 2017

Manual Tests for Windows x64 0.13.2 RC2 #7030

Closed

Manual Tests for Windows ia-32 0.13.2 RC3 #7029

Closed

Manual Tests for Linux 0.13.2 RC3 #7028

Closed

Manual Tests for OS X 0.13.2 RC2 #7027

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restyle preferences payments #6101

restyle preferences payments #6101

jkup commented Dec 8, 2016 •

edited by luixxiul

Loading

jkup commented Dec 9, 2016

bsclifton commented Dec 9, 2016 •

edited

Loading

luixxiul Dec 9, 2016

bsclifton commented Dec 9, 2016

jkup commented Dec 13, 2016

bsclifton commented Dec 14, 2016

luixxiul commented Dec 14, 2016

bsclifton commented Dec 14, 2016

jkup commented Dec 14, 2016

bsclifton commented Dec 15, 2016

jkup commented Dec 15, 2016

luixxiul Dec 16, 2016

luixxiul Dec 16, 2016

jkup commented Dec 16, 2016

luixxiul commented Dec 17, 2016

jkup commented Dec 17, 2016

luixxiul commented Dec 17, 2016

bsclifton commented Jan 9, 2017

luixxiul commented Jan 10, 2017 •

edited

Loading

bsclifton commented Jan 10, 2017 •

edited

Loading

luixxiul commented Jan 10, 2017

bsclifton commented Jan 10, 2017 •

edited

Loading

jkup commented Jan 11, 2017

luixxiul commented Jan 11, 2017 •

edited

Loading

bsclifton commented Jan 11, 2017

restyle preferences payments #6101

restyle preferences payments #6101

Conversation

jkup commented Dec 8, 2016 • edited by luixxiul Loading

jkup commented Dec 9, 2016

bsclifton commented Dec 9, 2016 • edited Loading

luixxiul Dec 9, 2016

Choose a reason for hiding this comment

bsclifton commented Dec 9, 2016

jkup commented Dec 13, 2016

bsclifton commented Dec 14, 2016

luixxiul commented Dec 14, 2016

bsclifton commented Dec 14, 2016

jkup commented Dec 14, 2016

bsclifton commented Dec 15, 2016

jkup commented Dec 15, 2016

luixxiul Dec 16, 2016

Choose a reason for hiding this comment

luixxiul Dec 16, 2016

Choose a reason for hiding this comment

jkup commented Dec 16, 2016

luixxiul commented Dec 17, 2016

jkup commented Dec 17, 2016

luixxiul commented Dec 17, 2016

bsclifton commented Jan 9, 2017

luixxiul commented Jan 10, 2017 • edited Loading

bsclifton commented Jan 10, 2017 • edited Loading

luixxiul commented Jan 10, 2017

bsclifton commented Jan 10, 2017 • edited Loading

jkup commented Jan 11, 2017

luixxiul commented Jan 11, 2017 • edited Loading

bsclifton commented Jan 11, 2017

jkup commented Dec 8, 2016 •

edited by luixxiul

Loading

bsclifton commented Dec 9, 2016 •

edited

Loading

luixxiul commented Jan 10, 2017 •

edited

Loading

bsclifton commented Jan 10, 2017 •

edited

Loading

bsclifton commented Jan 10, 2017 •

edited

Loading

luixxiul commented Jan 11, 2017 •

edited

Loading