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

Restructuring paymentsTab.js #7751

Merged
merged 1 commit into from
Apr 4, 2017
Merged

Restructuring paymentsTab.js #7751

merged 1 commit into from
Apr 4, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 16, 2017

Closes #7750

Auditors:

  • Removed 'pull-left', which is no longer used
  • Removed styles.titleBar
  • Created flexAlignCenter and flexAlignEnd as general properties
  • Replaced margin and padding with position:relative to set space between elements
  • Renamed advanceIcon to settingIcon
  • Changed the FAQ icons color to that on about pages (cf: about:preferences#plugins)

Also:

  • Added globalStyles.fontSize.settingItemSubtext to global.js
  • Added commonTextColor to preferences.less
  • Added globalStyles.color.commonTextColor to global.js
  • Added autoSuggestSwitch__subtext

Test Plan:

  1. Open about:preferences#payments
  2. Enable the payment
  3. Make sure the switches, their labels and the buttons are aligned horizontally
  4. Disable the payment
  5. Make sure the switches and the labels are aligned

screenshot 2017-04-04 18 18 26

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

Test Plan:

@luixxiul luixxiul self-assigned this Mar 16, 2017
@luixxiul luixxiul requested a review from NejcZdovc March 16, 2017 17:00
@luixxiul luixxiul added this to the 0.14.1 milestone Mar 17, 2017
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

image

This is how I see it

@luixxiul
Copy link
Contributor Author

It seems the upper line is a bit away. Please try to set it on "r", "m", "s" of "Brave payments", and the two switches.

@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 18, 2017

Mine is actually not a perfect one, though. This is the cropped image:
clipboard01

The height of the switches and the small characters, not the capital ones, should be the same, following the current status.

@NejcZdovc
Copy link
Contributor

image

I don't see it the same way as you do

@luixxiul
Copy link
Contributor Author

yeah I notice it as well, will fix soon.

@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 18, 2017

OK I slightly modified the positions.

screenshot 2017-03-18 17 46 21

I'm not a fan of positioning with pixels (as it is often not compatible with other languages in terms of l10n), still it works in English on macOS, for now.

IMO aligning all elements at center with align-items: center would be easier and more friendly with the languages. If this is better, I would add fixes. @bradleyrichter would you tell me what you think?

@bsclifton
Copy link
Member

@NejcZdovc now that your changes are merged, can you review this one?
@luixxiul can you add steps to the original post for a test plan?

Thanks 😄

@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 3, 2017

ok I will squash the commits at first.

@NejcZdovc NejcZdovc self-requested a review April 3, 2017 16:54
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 3, 2017

OK I added the test plan in the commit message and the 1st post.

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

image

Can you please move icons at the end for 1px up, other then that it looks good.

Closes #7750

- Removed 'pull-left', which is no longer used
- Removed styles.titleBar
- Created flexAlignCenter and flexAlignEnd as general properties
- Replaced margin and padding with position:relative to set space between elements
- Renamed advanceIcon to settingIcon

- Changed the FAQ icons color to that on about pages (cf: about:preferences#plugins)

Also:
- Added globalStyles.fontSize.settingItemSubtext to global.js
- Added commonTextColor to preferences.less
- Added globalStyles.color.commonTextColor to global.js
- Added autoSuggestSwitch__subtext

Test Plan:
1. Open about:preferences#payments
2. Enable the payment
3. Make sure the switches, their labels and the buttons are aligned horizontally
4. Disable the payment
5. Make sure the switches and the labels are aligned
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 4, 2017

Done. I restructured a little bit more. This should be fine.

screenshot 2017-04-04 18 18 26

@luixxiul luixxiul mentioned this pull request Apr 4, 2017
4 tasks
@NejcZdovc NejcZdovc self-requested a review April 4, 2017 10:48
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

LGTM, great job

@luixxiul luixxiul merged commit e382111 into brave:master Apr 4, 2017
@luixxiul luixxiul deleted the paymentUI-followup branch April 4, 2017 12:43
@luixxiul luixxiul mentioned this pull request Apr 11, 2017
4 tasks
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.

3 participants