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

Unify button style #1419

Merged
merged 8 commits into from
Dec 12, 2017
Merged

Unify button style #1419

merged 8 commits into from
Dec 12, 2017

Conversation

dominik-zeglen
Copy link
Contributor

@dominik-zeglen dominik-zeglen commented Dec 5, 2017

This PR unifies button style in dashboard. Changes flat button's background to gray (when focused), so it doesn't look like primary button when hovered over. Also applies minor hotfixes to UI.

zrzut ekranu 2017-12-05 o 14 35 55

zrzut ekranu 2017-12-05 o 14 36 11

zrzut ekranu 2017-12-05 o 14 36 36

zrzut ekranu 2017-12-05 o 14 36 59

@codecov-io
Copy link

codecov-io commented Dec 5, 2017

Codecov Report

Merging #1419 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1419   +/-   ##
=======================================
  Coverage   83.41%   83.41%           
=======================================
  Files         128      128           
  Lines        5795     5795           
  Branches      665      665           
=======================================
  Hits         4834     4834           
  Misses        803      803           
  Partials      158      158

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ee14e1...8f0434a. Read the comment docs.

@patrys
Copy link
Member

patrys commented Dec 5, 2017

This is not consistent with how Google uses material design. IMHO we should not be giving flat buttons a background at all (that's why they are flat). Why not increase the contrast of the text color instead?

@dominik-zeglen
Copy link
Contributor Author

Background appears only when button is focused - in its "idle" state it's really flat. Like 100% Material Design specification flat. Also, I followed guide lines from here:
https://material.io/guidelines/components/buttons.html#buttons-flat-buttons

@patrys
Copy link
Member

patrys commented Dec 5, 2017

Sorry, I thought you made the background grey on hover, not on focus.

&:hover {
background: $error-color;
color: #fff;
background: transparentize(#cccccc, .6);
Copy link
Member

@patrys patrys Dec 5, 2017

Choose a reason for hiding this comment

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

Wait, you do set background color on hover. This should be :focus, :active below is correct.

Copy link
Member

@patrys patrys left a comment

Choose a reason for hiding this comment

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

You've now changed :active and :focus to be identical but they're not supposed to be per spec.

Is there a color change on :hover for flat buttons?

@dominik-zeglen
Copy link
Contributor Author

No, reverted that. Also you are right, we should set in css only :focus, press effect is done by JS.

@maarcingebala maarcingebala merged commit acdd238 into master Dec 12, 2017
@maarcingebala maarcingebala deleted the fix/unify-buttons branch December 12, 2017 10:31
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.

None yet

4 participants