make button styles consistent #5916

jkup · 2016-11-29T20:54:33Z

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:

This commit changes a lot of our global button styles. The best way to test this would be to click through each of the settings pages and the corresponding dialogs and make sure the orange and white buttons we use look consistent.

Some specific examples to check:

Preferences > Security > Manage Autofill Data (the buttons on this page should no longer be bold)
Bookmarks > Edit Bookmark (when hovering on the Remove button the text should no longer turn white)
Preferences > General > Import now... (white button should not have orange text)

Here is a great screenshot @bsclifton put together that shows some inconsistencies. These buttons should all look the same now!

jkup · 2016-11-29T23:50:14Z

Two things I need to fix right now:

Brave lion in top right looks wrong (I removed a class)
Same thing for view all bookmarks button on add/edit bookmark dialog

luixxiul · 2016-11-30T01:14:44Z

The primaryButton seems to need more padding.

padding: .5rem 1rem; should work nice.

Another example

luixxiul · 2016-11-30T02:47:43Z

Also noScript button and buttons inside the noscript dialog should be fixed.

with border: none; background: none; on .browserButton

luixxiul · 2016-11-30T04:30:06Z

Ah, I think I've done too much here :-(

Maybe this PR could be changed to fix all of the button issues (actual/potential) as well as the styling, if you don't mind.

luixxiul · 2016-11-30T04:32:29Z

.menuButton is yet to be modified, just fyi

luixxiul · 2016-11-30T10:19:35Z

less/button.less

+    line-height: 1.25;
+    margin: 5px 5px 0px 0px;
+    padding-top: 5px;
+    padding-bottom: 5px;


1em=16px, 16px*0.3=4.8px≒5px

luixxiul · 2016-11-30T13:16:09Z

less/button.less

-    cursor: pointer;
-    outline: none;
-    text-align: left;
+// for about:preferences


The buttons under this section should be removed if necessary. especially from L171 to L176.

jkup · 2016-11-30T20:08:20Z

@luixxiul thanks for those changes!

luixxiul · 2016-12-01T02:40:47Z

~~Still we should address margin around the buttons because margin-top was removed from the general button class .browserButton. eg. about:autofill~~

Nevermind, the margin does not exist from the first.

luixxiul · 2016-12-01T07:57:48Z

less/about/preferences.less

@@ -15,6 +15,7 @@ body {

 #paymentsSwitches {
  margin-top: 15px;
+  padding: 5px;


The elements become unaligned.

luixxiul · 2016-12-01T08:08:40Z

less/about/preferences.less

-  margin-top: 15px;
+  display: flex;
+  align-items: center;
+  min-height: 40px;


Fixed into:

The label of the button "off" is slightly up. Will fix with another PR.

luixxiul · 2016-12-01T13:43:06Z

less/button.less

-    padding-left: 1em;
+// for about:preferences
+.prefBody {
+  .settingsList > .settingItem + button {


Because the button should be placed under <SettingCheckbox>. For example: https://github.com/brave/browser-laptop/pull/5916/files#diff-e3eeb751016b2ce9f8278efce585a461R1519

luixxiul · 2016-12-01T13:43:16Z

less/button.less

-    &:hover {
-      color: white;
-    }
+  .settingItem > span + button {


See:

browser-laptop/js/about/preferences.js

Lines 731 to 734 in a162181

<SettingItem dataL10nId='importBrowserData'>

<Button l10nId='importNow' className='primaryButton importNowButton'

onClick={this.importBrowserDataNow} />

</SettingItem>

luixxiul · 2016-12-01T13:49:51Z

less/about/preferences.less

-    &.setAsDefaultButton,
-    &.manageAdblockSettings,
-    &.viewExtensionsInfo,
-    &.manageAutofillDataButton {


These were replaced with .settingsList > .settingItem + button and .settingItem > span + button. Styling with classes in that way is a cause of inconsistency in both CSS and JS.

luixxiul · 2016-12-01T14:05:14Z

less/button.less

    font-size: 0.9em;
-    margin-top: 20px;
    padding: 5px 20px;


These values should be reconsidered later

luixxiul · 2016-12-01T14:33:00Z

The font-family of the buttons on about:preferences is Arial on common.less, while the global font-family is set to -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen, Ubuntu, Cantarell, "Fira Sans", "Droid Sans", "Helvetica Neue", sans-serif; on window.less. This causes another big inconsistency. How should we handle this? @bradleyrichter

bradleyrichter · 2016-12-01T14:59:03Z

Let's unify assuming the new button refinements will allow for the font flexibility.

…

On Dec 1, 2016, at 6:33 AM, Suguru Hirahara ***@***.***> wrote: The font-family of the buttons on about:preferences is Arial on common.less, while the global font-family is set to -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen, Ubuntu, Cantarell, "Fira Sans", "Droid Sans", "Helvetica Neue", sans-serif; on window.less. This causes another big inconsistency. How should we handle this? @bradleyrichter — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

@bradleyrichter

- some buttons on about:preferences were excluded from settingItem - the buttons styles on about pages were made consistent - about:preferences - about:history - elements in titleBar on about:preferences#payments were aligned - font-size and padding of the buttons on about:preferences were made consistent Auditors: @bradleyrichter Test Plan:

jkup · 2016-12-01T19:27:06Z

@luixxiul Where are we at on this one? I see you opened a lot of issues and added a lot of commits. Is this ready for review / merging in your opinion?

As for changing the font family in common.less, let's split that into another ticket if that's the approach we want to take. As it affects more than just buttons.

bsclifton · 2016-12-02T15:51:36Z

Looks good to me! Nice work 😄 I'll leave as-is right now, since it has "WiP" in the title, but if it's all done, let's go ahead and merge it, @jkup 😄

- Paddings of buttons on about:preferences were increased - Removed unnecessary class Auditors: Test Plan:

luixxiul · 2016-12-02T18:18:55Z

less/button.less

@@ -158,9 +158,10 @@ span.buttonSeparator {
 // for about:preferences
 .prefBody {
  .settingsList > .settingItem + button,
-  .settingItem > span + button {
+  .settingItem > span + button,
+  #paymentsContainer button:not(.close) {


This increases padding except the close "x" button on the payments dialog.

luixxiul · 2016-12-02T18:20:03Z

Increased and added padding of the buttons on about:preferences. I think this is ready for re-review.

bsclifton · 2016-12-02T21:23:08Z

Should be good to go 😄 Merging...

bradleyrichter · 2016-12-02T21:23:48Z

++, and +

bradleyrichter · 2016-12-03T03:31:24Z

@jkup @luixxiul I found a spacing issue that might be from this PR. Can you please investigate?

Left is master. Right is 0.12.11

luixxiul · 2016-12-03T03:38:30Z

@bradleyrichter I've noticed that and it should be fixed by adding margin-bottom to the wallet address. Will fix.

bradleyrichter · 2016-12-03T03:43:11Z

thanks!

luixxiul force-pushed the button-styles branch from d09c1aa to 8e5c3a1 Compare November 30, 2016 04:19

luixxiul force-pushed the button-styles branch from 3990a63 to accb4b5 Compare November 30, 2016 04:43

luixxiul reviewed Nov 30, 2016

View reviewed changes

luixxiul force-pushed the button-styles branch from 9f63cac to c85a637 Compare November 30, 2016 18:26

jkup force-pushed the button-styles branch from c85a637 to 834b6b2 Compare November 30, 2016 20:08

luixxiul reviewed Dec 1, 2016

View reviewed changes

luixxiul mentioned this pull request Dec 1, 2016

Replaced "clearBrowsingDataButton" to "importBrowserData" #5961

Merged

4 tasks

luixxiul reviewed Dec 1, 2016

View reviewed changes

less/button.less

font-size: 0.9em;

margin-top: 20px;

padding: 5px 20px;

Copy link

Contributor

luixxiul Dec 1, 2016

Choose a reason for hiding this comment

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

These values should be reconsidered later

luixxiul force-pushed the button-styles branch from ea1fb55 to 0be7105 Compare December 1, 2016 15:08

luixxiul changed the title ~~make button styles consistent~~ [WiP] make button styles consistent Dec 1, 2016

jkup mentioned this pull request Dec 1, 2016

Add focus ring to about page form elements #5971

Closed

jkup and others added 2 commits December 1, 2016 11:24

make button styles consistent

43e8923

jkup force-pushed the button-styles branch from 0be7105 to e8955ce Compare December 1, 2016 19:24

jkup mentioned this pull request Dec 1, 2016

Payment Button mouse over barely noticeable #4708

Closed

bsclifton assigned bradleyrichter Dec 2, 2016

Make button styles consistent (part 3)

46ee335

- Paddings of buttons on about:preferences were increased - Removed unnecessary class Auditors: Test Plan:

luixxiul force-pushed the button-styles branch from b52eb32 to 46ee335 Compare December 2, 2016 18:16

luixxiul reviewed Dec 2, 2016

View reviewed changes

bsclifton changed the title ~~[WiP] make button styles consistent~~ make button styles consistent Dec 2, 2016

bsclifton merged commit d3addcf into master Dec 2, 2016

bsclifton deleted the button-styles branch December 2, 2016 21:23

luixxiul added the refactoring label Dec 3, 2016

luixxiul mentioned this pull request Dec 3, 2016

Follow-up: Button styles consistent #6001

Merged

4 tasks

luixxiul added the misc/button label Dec 3, 2016

bradleyrichter mentioned this pull request Dec 3, 2016

Button tweak prefs font nicer #6000

Merged

luixxiul mentioned this pull request Dec 9, 2016

the buttons on about:preferences#payments need more paddings #6081

Closed

luixxiul mentioned this pull request Dec 20, 2016

Buttons alignment on about:certerror and NoScript dialog #6317

Closed

This was referenced Dec 21, 2016

Manual tests for Windows ia-32 0.13.0 Preview 6 #6335

Closed

Manual tests for Linux Mint 0.13.0 Preview 6 #6337

Closed

Manual tests for Windows x64 0.13.0 Preview 6 #6334

Closed

Manual tests for OS X 0.13.0 Preview 6 #6332

Closed

luixxiul added the QA/checked-Win32 label Dec 25, 2016

luixxiul mentioned this pull request Dec 30, 2016

"No saved addresses/credit cards" messages and the autofill list lack margins around them #6328

Closed

luixxiul mentioned this pull request Jan 16, 2017

Make button styles consistent #5801

Closed

luixxiul removed the QA/checked-Win32 label Jan 16, 2017

This was referenced Jan 16, 2017

Tracking: follow-up button styles consistent #6675

Closed

No-script notification buttons need margins #6783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make button styles consistent #5916

make button styles consistent #5916

jkup commented Nov 29, 2016 •

edited

Loading

jkup commented Nov 29, 2016

luixxiul commented Nov 30, 2016 •

edited

Loading

luixxiul commented Nov 30, 2016 •

edited

Loading

luixxiul commented Nov 30, 2016 •

edited

Loading

luixxiul commented Nov 30, 2016

luixxiul Nov 30, 2016

luixxiul Nov 30, 2016

jkup commented Nov 30, 2016

luixxiul commented Dec 1, 2016 •

edited

Loading

luixxiul Dec 1, 2016 •

edited

Loading

luixxiul Dec 1, 2016 •

edited

Loading

luixxiul Dec 1, 2016

luixxiul Dec 1, 2016

luixxiul Dec 1, 2016 •

edited

Loading

luixxiul Dec 1, 2016

luixxiul commented Dec 1, 2016

bradleyrichter commented Dec 1, 2016 via email

jkup commented Dec 1, 2016

bsclifton commented Dec 2, 2016

luixxiul Dec 2, 2016

luixxiul commented Dec 2, 2016

bsclifton commented Dec 2, 2016

bradleyrichter commented Dec 2, 2016

bradleyrichter commented Dec 3, 2016

luixxiul commented Dec 3, 2016

bradleyrichter commented Dec 3, 2016

	<SettingItem dataL10nId='importBrowserData'>
	<Button l10nId='importNow' className='primaryButton importNowButton'
	onClick={this.importBrowserDataNow} />
	</SettingItem>

make button styles consistent #5916

make button styles consistent #5916

Conversation

jkup commented Nov 29, 2016 • edited Loading

jkup commented Nov 29, 2016

luixxiul commented Nov 30, 2016 • edited Loading

luixxiul commented Nov 30, 2016 • edited Loading

luixxiul commented Nov 30, 2016 • edited Loading

luixxiul commented Nov 30, 2016

luixxiul Nov 30, 2016

Choose a reason for hiding this comment

luixxiul Nov 30, 2016

Choose a reason for hiding this comment

jkup commented Nov 30, 2016

luixxiul commented Dec 1, 2016 • edited Loading

luixxiul Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

luixxiul Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

luixxiul Dec 1, 2016

Choose a reason for hiding this comment

luixxiul Dec 1, 2016

Choose a reason for hiding this comment

luixxiul Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

luixxiul Dec 1, 2016

Choose a reason for hiding this comment

luixxiul commented Dec 1, 2016

bradleyrichter commented Dec 1, 2016 via email

jkup commented Dec 1, 2016

bsclifton commented Dec 2, 2016

luixxiul Dec 2, 2016

Choose a reason for hiding this comment

luixxiul commented Dec 2, 2016

bsclifton commented Dec 2, 2016

bradleyrichter commented Dec 2, 2016

bradleyrichter commented Dec 3, 2016

luixxiul commented Dec 3, 2016

bradleyrichter commented Dec 3, 2016

jkup commented Nov 29, 2016 •

edited

Loading

luixxiul commented Nov 30, 2016 •

edited

Loading

luixxiul commented Nov 30, 2016 •

edited

Loading

luixxiul commented Nov 30, 2016 •

edited

Loading

luixxiul commented Dec 1, 2016 •

edited

Loading

luixxiul Dec 1, 2016 •

edited

Loading

luixxiul Dec 1, 2016 •

edited

Loading

luixxiul Dec 1, 2016 •

edited

Loading