Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix BEM naming and align elements correctly on paymentsTab.js and ledgerTable.js #10913

Merged
merged 2 commits into from
Nov 24, 2017
Merged

Fix BEM naming and align elements correctly on paymentsTab.js and ledgerTable.js #10913

merged 2 commits into from
Nov 24, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Sep 12, 2017

Note: if this PR will be postponed to 0.22.x or later, please do not forget to merge #11368 to 0.21.x. If not, the switch placement will regress.

Closes #10912

  • Disable line wrap on hideExcludedSites__settingCheckbox
  • Update to our modified BEM style (paymentsTab.js)
  • Indentation

Auditors:

Test Plan:

  1. Run npm run add-simulated-synopsis-visits 100
  2. Run npm run add-simulated-payment-history
  3. Restart the browser
  4. Open about:preferences#payments
  5. Make sure the layout is not broken
  6. Make sure Show only included sites label is not wrapped
  7. Make sure only site domains are clickable

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

@codecov-io
Copy link

codecov-io commented Sep 12, 2017

Codecov Report

Merging #10913 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10913      +/-   ##
==========================================
- Coverage   53.47%   53.45%   -0.02%     
==========================================
  Files         275      275              
  Lines       26042    26033       -9     
  Branches     4180     4180              
==========================================
- Hits        13926    13916      -10     
- Misses      12116    12117       +1
Flag Coverage Δ
#unittest 53.45% <100%> (-0.02%) ⬇️
Impacted Files Coverage Δ
app/renderer/components/styles/global.js 100% <ø> (ø) ⬆️
...r/components/preferences/payment/enabledContent.js 87.21% <100%> (-1.44%) ⬇️
...erer/components/preferences/payment/ledgerTable.js 89.52% <100%> (-0.06%) ⬇️
app/renderer/components/preferences/paymentsTab.js 75% <100%> (ø) ⬆️

@luixxiul luixxiul added this to the 0.21.x (Nightly Channel) milestone Sep 13, 2017
@luixxiul luixxiul added the priority/P4 Minor loss of function. Workaround usually present. label Sep 13, 2017
cezaraugusto
cezaraugusto previously approved these changes Sep 19, 2017
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

lgtm but delaying merge to be after #10953 which will land in master first. Setting as blocked for now

@cezaraugusto
Copy link
Contributor

blocked as waiting for #10953

@cezaraugusto
Copy link
Contributor

also please rebase

@ghost ghost removed the priority/P5 Cosmetic. Spelling, copy, layout. New features (which should also be part of an initiative). label Sep 26, 2017
@luixxiul luixxiul dismissed cezaraugusto’s stale review October 7, 2017 19:03

Rebased on the recent BAT update

@luixxiul luixxiul mentioned this pull request Oct 8, 2017
8 tasks
@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 9, 2017

I tried something like this to align switches on macOS (en-US / en-UK).

--- a/app/renderer/components/preferences/paymentsTab.js
+++ b/app/renderer/components/preferences/paymentsTab.js
@@ -39,6 +39,8 @@ const batIcon = require('../../../extensions/brave/img/ledger/cryptoIcons/BAT_ic
 // other
 const getSetting = require('../../../../js/settings').getSetting
 const settings = require('../../../../js/constants/settings')
+const platformUtil = require('../../../common/lib/platformUtil')
+const isDarwin = platformUtil.isDarwin()
 const {formatCurrentBalance, batToCurrencyString} = require('../../../common/lib/ledgerUtil')
 
 class PaymentsTab extends ImmutableComponent {
@@ -107,6 +109,7 @@ class PaymentsTab extends ImmutableComponent {
 
   render () {
     const enabled = this.props.ledgerData.get('created')
+    const isEnglish = getSetting(settings.LANGUAGE, this.props.settings) === 'en_US' || 'en_UK'
 
     return <div className={cx({
       paymentsContainer: true,
@@ -218,11 +221,13 @@ class PaymentsTab extends ImmutableComponent {
               switchClassName={css(styles.switch__switchControl)}
               leftLabelClassName={css(
                 styles.switch__label,
+                (isDarwin && isEnglish) && [styles.switch__label_isDarwin_isEnglish, styles.switch__label_isDarwin_isEnglish_left],

Could I know how to fix this one?

@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel), Backlog Oct 25, 2017
@luixxiul luixxiul added PR/pending-review QA/test-plan-specified and removed priority/P4 Minor loss of function. Workaround usually present. labels Oct 29, 2017
@luixxiul luixxiul added the 0.21.x issue first seen in 0.21.x label Nov 15, 2017
@luixxiul luixxiul changed the title Polish paymentsTab.js and ledgerTable.js Fix BEM naming and align elements correctly on paymentsTab.js and ledgerTable.js Nov 17, 2017
@cezaraugusto
Copy link
Contributor

re: #10913 (comment) why is that a thing in English only? or is the comment superseded by newer commits?

@cezaraugusto cezaraugusto added 0.22.x issue first seen in 0.22.x and removed PR/blocked 0.21.x issue first seen in 0.21.x labels Nov 21, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Nov 21, 2017

@cezaraugusto to fix the issue here: #10482 (comment)

TODO: Apply 'position: relative' and 'bottom: 1px' for macOS (en_US) only on about:preferences#payments.

The placement of the right label has been corrupted on the other environments such as Windows 10 (ja-JP);
clipboard01

That modification is currently required only for en_US and en_UK on macOS.

It might be required for Windows and Linux, if the switch label and the info button are not aligned properly there as well.

Suguru Hirahara added 2 commits November 21, 2017 19:11
- Disable line wrap on 'hideExcludedSites__settingCheckbox'
- Fix BEM naming
- Make only text of anchor links clickable

Auditors:

Test Plan:
1. Run `npm run add-simulated-synopsis-visits 100`
2. Restart the browser
3. Open about:preferences#payments
4. Make sure `Show only included sites` label is not wrapped
5. Make sure only site domains are clickable
- Fixes BEM naming
- Indentation
- Replace padding inside .walletBar with margin inside its elements

Closes #10912

Auditors:

Test Plan:
1. Run `npm run add-simulated-payment-history`
2. Restart the browser
3. Open about:preferences#payments
4. Make sure the layout is not broken
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

++

@cezaraugusto cezaraugusto merged commit 624869d into brave:master Nov 24, 2017
@luixxiul luixxiul deleted the polish-paymentsTab branch November 24, 2017 04:20
@luixxiul luixxiul modified the milestones: Triage Backlog, 0.22.x (Nightly Channel) Nov 24, 2017
@bbondy bbondy modified the milestones: 0.22.x (Developer Channel), 0.23.x (Nightly Channel) Feb 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants