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

Update enabledContent.js to BEM style #10147

Closed
wants to merge 1 commit into from
Closed

Update enabledContent.js to BEM style #10147

wants to merge 1 commit into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Jul 26, 2017

Closes #10146

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:

  1. Open about:preferences#payments
  2. Enable Payments
  3. Make sure the page is loaded without breaking styles

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 luixxiul added the polish Nice to have — usually related to front-end/visual tasks. label Jul 26, 2017
@luixxiul luixxiul added this to the 0.20.x (Nightly Channel) milestone Jul 26, 2017
@luixxiul luixxiul self-assigned this Jul 26, 2017
@luixxiul luixxiul requested a review from cezaraugusto July 26, 2017 09:08
@luixxiul
Copy link
Contributor Author

note: I will squash the commits in one commit after review is done.

@luixxiul
Copy link
Contributor Author

addFunds(https://github.com/brave/browser-laptop/pull/10147/files#diff-5d8199641ea1ba12e0394859f7eb0c55R282) is not edited as it eventually will be removed with #10142.

marginTop: '15px',
lineHeight: 1.5
},

settingsListContainer: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

settingsListContainer is not added to any elements so this can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to remove

@codecov-io
Copy link

codecov-io commented Jul 26, 2017

Codecov Report

Merging #10147 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #10147   +/-   ##
=======================================
  Coverage   52.93%   52.93%           
=======================================
  Files         228      228           
  Lines       20297    20297           
  Branches     3252     3252           
=======================================
  Hits        10744    10744           
  Misses       9553     9553
Flag Coverage Δ
#unittest 52.93% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...r/components/preferences/payment/enabledContent.js 93.44% <100%> (ø) ⬆️

@@ -3,7 +3,7 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const React = require('react')
const {StyleSheet, css} = require('aphrodite')
const {StyleSheet, css} = require('aphrodite/no-important')
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

textDecoration: 'none',

':hover': {
textDecoration: 'none'
}
},

iconText: {
balance__iconLink__iconText: {
color: globalStyles.color.mediumGray,
margin: '0 0 0 5px',

Copy link
Contributor

@cezaraugusto cezaraugusto Aug 1, 2017

Choose a reason for hiding this comment

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

below we have:

// TODO: refactor preferences.less to remove !important
fontSize: `${globalStyles.fontSize.settingItemSubtext} !important

is that still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually paymentStyles.font.regular should be replaced with globalStyles.fontSize.settingItemSubtext as there is no reason why font-size on the payments tab is different than the other tabs.

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.

found one visual regression, other than that lgtm:

screen shot 2017-07-31 at 11 13 09 pm

@luixxiul
Copy link
Contributor Author

luixxiul commented Aug 1, 2017

The regression was fixed with 72c9fc1.

Closes #10146

Also:
- aphrodite -> aphrodite/no-important
@luixxiul
Copy link
Contributor Author

luixxiul commented Aug 1, 2017

The commits were squashed and the PR is ready to be merged.

@luixxiul
Copy link
Contributor Author

luixxiul commented Aug 1, 2017

I'm restructuring enabledContent.js with grid, replacing flex with it, and most of the change of this PR will be replaced. So I'm going to close this PR.

@luixxiul luixxiul closed this Aug 1, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Aug 1, 2017

Opened: #10238

@luixxiul luixxiul deleted the polish-bem-enabledContent branch August 1, 2017 18:46
@luixxiul luixxiul removed this from the 0.20.x (Developer Channel) milestone Aug 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature/about-pages polish Nice to have — usually related to front-end/visual tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants