Skip to content
This repository has been archived by the owner. It is now read-only.

Move the switch wrappers 1rem left #11368

Merged
merged 1 commit into from Oct 9, 2017
Merged

Conversation

@luixxiul
Copy link
Contributor

luixxiul commented Oct 8, 2017

This is required for 0.19.x and 0.20.x only for now.

screenshot 2017-10-09 1 00 30

Fixes #11367

Auditors:

Test Plan:

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
Suguru Hirahara
Fixes #11367

Auditors:

Test Plan:
@luixxiul luixxiul added this to the 0.19.x (Beta Channel) milestone Oct 8, 2017
@luixxiul luixxiul self-assigned this Oct 8, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 8, 2017

To merger: please do not forget to merge this commit to 0.19.x as well, thanks! It is not necessary to merge this to master as the whole style block will be replaced with #10913 anyway

@codecov-io
Copy link

codecov-io commented Oct 8, 2017

Codecov Report

Merging #11368 into 0.20.x will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           0.20.x   #11368      +/-   ##
==========================================
- Coverage   50.73%   50.69%   -0.04%     
==========================================
  Files         248      248              
  Lines       23598    23598              
  Branches     3872     3872              
==========================================
- Hits        11972    11963       -9     
- Misses      11626    11635       +9
Flag Coverage Δ
#unittest 50.69% <ø> (-0.04%) ⬇️
Impacted Files Coverage Δ
app/renderer/components/preferences/paymentsTab.js 76% <ø> (ø) ⬆️
js/stores/appStoreRenderer.js 91.17% <0%> (-8.83%) ⬇️
app/renderer/components/reduxComponent.js 84.37% <0%> (-6.25%) ⬇️
js/stores/windowStore.js 27.14% <0%> (-0.32%) ⬇️

// TODO: Remove this when #10913 is merged to master
position: 'relative',
left: '1rem'

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Oct 9, 2017

Contributor

why is relative unit preferred?

This comment has been minimized.

Copy link
@luixxiul

luixxiul Oct 9, 2017

Author Contributor

it's a magic number. It should work for now on 0.19 and 0.20. It'll be removed since 0.21 so an exact calculation is not needed as far as it works.

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Oct 9, 2017

Contributor

ok thanks

@@ -365,8 +369,10 @@ const styles = StyleSheet.create({
// History and settings icons
switchWrap__mainIconsRight: {
position: 'relative',
right: '12px',
top: '3.5px'
top: '3.5px',

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Oct 9, 2017

Contributor

I know it was like this before does but this extra .5px makes any difference?

This comment has been minimized.

Copy link
@luixxiul

luixxiul Oct 9, 2017

Author Contributor

I remember I used a screen ruler and adding it made it look nicer a bit.

Copy link
Contributor

cezaraugusto left a comment

++

@cezaraugusto cezaraugusto merged commit f0ce375 into brave:0.20.x Oct 9, 2017
1 of 3 checks passed
1 of 3 checks passed
codecov/project 50.69% (-0.04%) compared to b7c2257
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codecov/patch Coverage not affected when comparing b7c2257...1429266
Details
cezaraugusto added a commit that referenced this pull request Oct 9, 2017
Move the switch wrappers 1rem left
@cezaraugusto
Copy link
Contributor

cezaraugusto commented Oct 9, 2017

master n/a
0.20.x f0ce375
0.19.x 7d68d48

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.