Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paper button styles #2669

Closed
wants to merge 0 commits into from
Closed

Paper button styles #2669

wants to merge 0 commits into from

Conversation

rossmoody
Copy link
Contributor

Fixes: brave/brave-browser#3633
Fixes: brave/brave-browser#4852

Before

Screen Shot 2019-06-11 at 11 28 49 PM

Screen Shot 2019-06-11 at 11 28 31 PM

After

example2
example

Test Plan:

  1. Go to settings
  2. Ensure the buttons in setting match button spec
  3. Open the print dialog with File -> Print
  4. Ensure the buttons are styled to spec.

The buttons effected by these style updates live in

  • Settings panels (not Brave Rewards)
  • Print modal window
  • Dialogs within settings

@bsclifton
Copy link
Member

Woah- this looks amazing ❤️ Awesome job, @rossmoody 😄 Will try to review soon...

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Looks good, almost there - just need some css variable name clearing up

border-color: currentColor !important;

--flat-focus-shadow-color: rgba(255, 69, 48, .4) !important;
--type-color: #3b3e4f; /* grey800 */
Copy link
Member

Choose a reason for hiding this comment

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

Is this a variable name we're making here? If so please prefix with brave- otherwise we may conflict with chromium (these variable names are global).
Same for default-border.

Anything without a prefix should be something defined in the chromium webui codebase that we want to override.
Anything with a prefix like paper-button- will be something defined in a specific paper (or other) library component that we want to override.

Apologies that there's no docs yet for this.

@@ -7,73 +7,53 @@
/* Refer to:
* ui/webui/resources/cr_elements/paper_button_style_css.html
* for styles to override.
*
Copy link
Member

Choose a reason for hiding this comment

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

Is there no longer a translation? We're using action-button still, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update paper buttons to spec Focus ring on buttons looks odd
3 participants