Restructure paymentsTab with grid property #10238

luixxiul · 2017-08-01T17:59:05Z

Closes #10236

Replace display:table with display:grid to align elements

Also:

Update enabledContent.js to BEM style (Closes Fix BEM naming on enabledContent.js #10146)
Add PanelDropdown
Remove paymentStyles from payment for now (until conversion is done)
Add Panel Dropdown to styles.js (Closes Add Panel Dropdown to styles.js #10245)

Test Plan 1:

Open about:preferences#payments
Enable payments
Make sure the elements are aligned equally on both rows and columns
Disable payments
Make sure the switch is placed on the same place

Test Plan 2:

Open about:styles#dropdowns
Make sure PanelDropdown appears there

If this does not look changed, nothing is broken :-)

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-08-01T18:00:55Z

I added gridStyles in the same way as I created the compact bravery panel.

luixxiul · 2017-08-01T18:02:11Z

app/renderer/components/preferences/payment/enabledContent.js

-        </table>
+        <div className={css(gridStyles.row1col1, styles.walletBar__title)} data-l10n-id='monthlyBudget' />
+        <div className={css(gridStyles.row1col2, styles.walletBar__title)} data-l10n-id='accountBalance' />
+        <div className={css(gridStyles.row1col3)} />


This is the blank item.

codecov-io · 2017-08-01T19:06:01Z

Codecov Report

Merging #10238 into master will increase coverage by 0.01%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master   #10238      +/-   ##
==========================================
+ Coverage   54.15%   54.16%   +0.01%     
==========================================
  Files         247      247              
  Lines       21575    21579       +4     
  Branches     3344     3346       +2     
==========================================
+ Hits        11684    11689       +5     
+ Misses       9891     9890       -1

Flag	Coverage Δ
#unittest	`54.16% <97.05%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
app/renderer/components/styles/global.js	`100% <ø> (ø)`	⬆️
app/renderer/components/styles/payment.js	`100% <ø> (ø)`	⬆️
app/renderer/components/common/browserButton.js	`69.04% <ø> (ø)`	⬆️
.../components/preferences/payment/disabledContent.js	`100% <ø> (ø)`	⬆️
...r/components/preferences/payment/enabledContent.js	`94.06% <100%> (+0.84%)`	⬆️
app/renderer/components/preferences/paymentsTab.js	`83.87% <100%> (+0.35%)`	⬆️
...erer/components/preferences/payment/ledgerTable.js	`89.67% <100%> (+0.17%)`	⬆️
app/renderer/components/common/dropdown.js	`90.62% <75%> (+0.62%)`	⬆️
... and 2 more

cezaraugusto · 2017-08-10T15:43:34Z

app/renderer/components/common/dropdown.js

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

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


you're actually passing a string here, so this is actually truthy and not true. For such cases you'd need to pass as a boolean like data-isPanel={true}, but standard will complain. Hidden values are evaluated to true anyway and make standard happy, so <FormDropdown data-isPanel {...this.props} /> is the right take.

<FormDropdown data-isPanel {...this.props} /> is the right take.

OK I'm changing.

Fixed with 91de03c#diff-b906e73e687bce1b6e897d2b8d80089bR44

cezaraugusto · 2017-08-10T15:44:29Z

app/renderer/components/common/dropdown.js

+
+  settings_panel: {
+    // ref: browserButton_panelItem
+    width: '180px'


if browserButton_panelItem shares the same value then it should be both hosted in global.js, then the comment can be removed

Fixed with:
91de03c#diff-b906e73e687bce1b6e897d2b8d80089bR89
91de03c#diff-f14732b3ea914a26c2215676e7208907R310

cezaraugusto · 2017-08-10T15:48:58Z

app/renderer/components/preferences/payment/disabledContent.js

@@ -59,7 +58,7 @@ const styles = StyleSheet.create({
    backgroundColor: globalStyles.color.lightGray,
    borderRadius: globalStyles.radius.borderRadiusUIbox,
    padding: '40px',
-    fontSize: paymentStyles.font.regular,
+    fontSize: globalStyles.payments.fontSize.regular,


why are we changing styles here?

To remove paymentsStyles for now. It is not maintained well and moving the values to global.js makes it the code much more understandable.

I'm going to re-introduce paymentStyles after files related with about:preferences#payments are polished and the other stuff (which could be related with them) on other files than ones under payment is also converted to normalize the common styles, if any.

If paymentStyles would be left, there would remain only this line as other two will be removed. It does not make much sense to have a const for one style, IMO.

ok I'm against payments.js styles anyway so doing what you did is better up to a point that we remove that file.

cezaraugusto · 2017-08-10T15:49:37Z

app/renderer/components/preferences/payment/enabledContent.js

@@ -84,11 +83,10 @@ class EnabledContent extends ImmutableComponent {

    return <section className={css(styles.balance)}>
      <FormTextbox data-test-id='fundsAmount' readOnly value={btcToCurrencyString(value, ledgerData)} />
-      <a className={css(styles.iconLink)} href='https://brave.com/Payments_FAQ.html' target='_blank'>
+      <a className={css(styles.balance__iconLink)} href='https://brave.com/Payments_FAQ.html' target='_blank'>


++ much better

Added rel='noopener' as well:
91de03c#diff-5d8199641ea1ba12e0394859f7eb0c55R86

cezaraugusto · 2017-08-10T15:51:47Z

app/renderer/components/preferences/payment/enabledContent.js

-    }
+const styles = StyleSheet.create({
+  walletBar: {
+    display: 'grid',


Grid rules 🦁

luixxiul · 2017-08-10T16:08:18Z

app/renderer/components/styles/global.js

@@ -327,6 +328,13 @@ const globalStyles = {
        background: '#ccc'
      }
    }
+  },
+
+  // TODO (Suguru): move them to payment.js after style refactoring is done


@cezaraugusto this TODO will be revisited.

ok thanks. also pls move things out of that most you can. I think that file should be removed

I see. I'm going to address that on another PR (as it is out of scope of this PR and it'll take some time).

cezaraugusto

great work, thanks ++

cezaraugusto · 2017-09-04T18:19:27Z

I tested before the conflict, can you rebase please? we can merge after that

luixxiul · 2017-09-06T11:42:39Z

@cezaraugusto rebased. this is ready for re-review, thanks.

NejcZdovc · 2017-09-11T06:37:15Z

@luixxiul can you rebase again pls

@cezaraugusto

Closes #10236 Replace display:table with display:grid to align elements Also: - Update enabledContent.js to BEM style (Closes #10146) - Add PanelDropdown - Remove paymentStyles from payment for now (until conversion is done) - Add Panel Dropdown to styles.js (Closes #10245) Auditors: @cezaraugusto Test Plan 1: 1. Open `about:preferences#payments` 2. Enable payments 3. Make sure the elements are aligned equally on both rows and columns 4. Disable payments 5. Make sure the switch is placed on the same place Test Plan 2: 1. Open about:styles#dropdowns 2. Make sure PanelDropdown appears there

srirambv · 2017-12-21T13:28:45Z

@luixxiul is this expected ?

luixxiul · 2017-12-21T17:04:13Z

the right column should be aligned to the gray box. something must have been fallen out during 0.19 series. will check out.

luixxiul added feature/about-pages polish Nice to have — usually related to front-end/visual tasks. refactoring/aphrodite labels Aug 1, 2017

luixxiul added this to the 0.21.x (Nightly Channel) milestone Aug 1, 2017

luixxiul self-assigned this Aug 1, 2017

luixxiul requested review from cezaraugusto and bsclifton August 1, 2017 17:59

luixxiul commented Aug 1, 2017

View reviewed changes

luixxiul mentioned this pull request Aug 1, 2017

Update enabledContent.js to BEM style #10147

Closed

8 tasks

luixxiul mentioned this pull request Aug 2, 2017

Add Panel Dropdown to styles.js #10245

Closed

luixxiul added the PR/needs-rebase ۞ label Aug 9, 2017

luixxiul removed request for cezaraugusto and bsclifton August 9, 2017 15:41

luixxiul removed the PR/needs-rebase ۞ label Aug 9, 2017

luixxiul requested review from cezaraugusto and bsclifton August 9, 2017 16:59

cezaraugusto reviewed Aug 10, 2017

View reviewed changes

luixxiul mentioned this pull request Aug 10, 2017

Refactor table.sortableTable #10265

Merged

8 tasks

luixxiul commented Aug 10, 2017

View reviewed changes

luixxiul requested review from cezaraugusto and removed request for bsclifton and cezaraugusto August 10, 2017 16:39

luixxiul added PR/work-in-progress ⚒ and removed PR/work-in-progress ⚒ labels Aug 15, 2017

luixxiul requested a review from cezaraugusto August 15, 2017 05:47

cezaraugusto approved these changes Sep 4, 2017

View reviewed changes

cezaraugusto added the PR/needs-rebase ۞ label Sep 4, 2017

luixxiul mentioned this pull request Sep 6, 2017

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10300

Closed

8 tasks

luixxiul removed the PR/needs-rebase ۞ label Sep 6, 2017

luixxiul requested a review from cezaraugusto September 6, 2017 11:41

NejcZdovc added the PR/needs-rebase ۞ label Sep 11, 2017

luixxiul removed the PR/needs-rebase ۞ label Sep 11, 2017

cezaraugusto merged commit 415619e into brave:master Sep 11, 2017

luixxiul deleted the polish-grid-paymentsTab branch September 11, 2017 10:48

This was referenced Sep 11, 2017

Fix BEM naming on enabledContent.js #10146

Closed

Restructure paymentsTab with grid property #10236

Closed

History limit in the state #10895

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure paymentsTab with grid property #10238

Restructure paymentsTab with grid property #10238

luixxiul commented Aug 1, 2017 •

edited

Loading

luixxiul commented Aug 1, 2017

luixxiul Aug 1, 2017 •

edited

Loading

codecov-io commented Aug 1, 2017 •

edited

Loading

cezaraugusto Aug 10, 2017

luixxiul Aug 10, 2017

luixxiul Aug 10, 2017

cezaraugusto Aug 10, 2017

luixxiul Aug 10, 2017

cezaraugusto Aug 10, 2017

luixxiul Aug 10, 2017 •

edited

Loading

luixxiul Aug 10, 2017

cezaraugusto Aug 10, 2017

cezaraugusto Aug 10, 2017

luixxiul Aug 10, 2017

cezaraugusto Aug 10, 2017

luixxiul Aug 10, 2017

luixxiul Aug 10, 2017

cezaraugusto Aug 10, 2017

luixxiul Aug 10, 2017

cezaraugusto left a comment

cezaraugusto commented Sep 4, 2017

luixxiul commented Sep 6, 2017

NejcZdovc commented Sep 11, 2017

srirambv commented Dec 21, 2017

luixxiul commented Dec 21, 2017

Restructure paymentsTab with grid property #10238

Restructure paymentsTab with grid property #10238

Conversation

luixxiul commented Aug 1, 2017 • edited Loading

luixxiul commented Aug 1, 2017

luixxiul Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Aug 1, 2017 • edited Loading

Codecov Report

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

luixxiul Aug 10, 2017 • edited Loading

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 left a comment

Choose a reason for hiding this comment

cezaraugusto commented Sep 4, 2017

luixxiul commented Sep 6, 2017

NejcZdovc commented Sep 11, 2017

srirambv commented Dec 21, 2017

luixxiul commented Dec 21, 2017

luixxiul commented Aug 1, 2017 •

edited

Loading

luixxiul Aug 1, 2017 •

edited

Loading

codecov-io commented Aug 1, 2017 •

edited

Loading

luixxiul Aug 10, 2017 •

edited

Loading