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

[Button] Refactor button gradients #1392

Merged
merged 4 commits into from
Jul 28, 2017
Merged

Conversation

llorca
Copy link
Contributor

@llorca llorca commented Jul 27, 2017

Follow up from #1367

Changes proposed in this pull request:

  • Remove linear-gradient-with-fallback mixin and rely on straight CSS
  • Gradients on buttons are !default variables that can be customized like all the other ones

@llorca llorca requested review from cmslewis and giladgray July 27, 2017 01:14
@llorca llorca force-pushed the al/button-background-image branch from d7b8535 to b6d21c4 Compare July 27, 2017 01:16
@blueprint-bot
Copy link

Refactor button gradients

Preview: documentation
Coverage: core | datetime

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

noticed a very subtle change to default button styles--they're flatter now. (intent buttons look great.)

before:
image

after:
image

box-shadow: $button-intent-box-shadow;
background-color: $hover-color;
background-image: $button-intent-gradient;
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to repeat same background-image in pseudo states like :hover (compare to line 165)?
in devtools, it seems safe to remove.

this comment applies several times throughout the changed files, i'm just gonna leave this one comment for them all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, looks fine

@blueprint-bot
Copy link

Remove unnecessary background-image on hover

Preview: documentation
Coverage: core | datetime

@pkwi
Copy link
Contributor

pkwi commented Jul 27, 2017

I'm not sure about the new flatter buttons. Any chance we can reproduce the old gradient style?

@llorca
Copy link
Contributor Author

llorca commented Jul 27, 2017

@pkwi that's just a refactor, not a change. Buttons didn't get flatter :)

Edit: just realizing that default buttons used to have a different gradient than default button hovered... On it with Piotr offline for a clarification

@llorca
Copy link
Contributor Author

llorca commented Jul 27, 2017

With 70% opacity for $button-gradient, here's what we get:
screen shot 2017-07-27 at 12 42 23 pm

Compare to before:
screen shot 2017-07-27 at 12 42 18 pm

Seems pretty legit, gonna push a commit now and wait for @pkwi confirmation tomorrow.

@blueprint-bot
Copy link

70% gradient opacity for default light button

Preview: documentation
Coverage: core | datetime

@blueprint-bot
Copy link

Bump default gradient to 80%

Preview: documentation | table
Coverage: core | datetime

@pkwi
Copy link
Contributor

pkwi commented Jul 28, 2017

80% looks good to me.

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

this is awesome

Copy link
Contributor

@cmslewis cmslewis left a comment

Choose a reason for hiding this comment

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

Everything affected in the refactor seems good to me. what about gradients in menus, text inputs, etc? Any plan there?

@giladgray
Copy link
Contributor

menus and inputs don't have background gradients, they have box-shadows. so we're 🆒

@giladgray giladgray merged commit f486a0d into master Jul 28, 2017
@giladgray giladgray deleted the al/button-background-image branch July 28, 2017 21:55
@omeid
Copy link

omeid commented Jul 30, 2017

A bit late to the party, but perhaps the $button-gradient, $button-intent-gradient, and $dark-button-gradient could be called -background instead of -gradient?

@giladgray
Copy link
Contributor

-gradient is accurate because it is used as background-image (one of many background sub-properties) and the value is actually a gradient, not any of the other valid background values.

We name our sass variables for their type: -color, -size, etc. So -gradient is consistent, but -background is not.

@omeid
Copy link

omeid commented Jul 30, 2017

Right, that is a good point! I overlooked that there was also background-color, perhaps -background-image? to along with button-background-color and so? It just that gradient describes the content of the variable (the details about the background image being a gradient) instead of what, which is a reference to an image used for background.

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.

6 participants