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

add new button variant #193

Merged
merged 9 commits into from
Dec 5, 2015
Merged

add new button variant #193

merged 9 commits into from
Dec 5, 2015

Conversation

JefMari
Copy link
Contributor

@JefMari JefMari commented Nov 24, 2015

For issue #162
image

@JefMari
Copy link
Contributor Author

JefMari commented Nov 24, 2015

Please Check @srph

@srph
Copy link
Contributor

srph commented Nov 24, 2015

-primary-link has a border. It's the -default with a primary-light color.

Also, does -primary-link fit? cc @maebe22 @vistajess @juztinlazaro

@JefMari
Copy link
Contributor Author

JefMari commented Nov 25, 2015

Border applied!
image

Question: How about its hover property? currently it also adopts default button hover property

@srph
Copy link
Contributor

srph commented Nov 25, 2015

Ask a designer. IMO, darken the text.

@JefMari
Copy link
Contributor Author

JefMari commented Nov 26, 2015

Update!
image

@srph
Copy link
Contributor

srph commented Nov 26, 2015

Was the darkened bg intentional?

@JefMari
Copy link
Contributor Author

JefMari commented Nov 26, 2015

nah, the text color only darken, though the bg is inherited from default

@srph
Copy link
Contributor

srph commented Nov 26, 2015

Can you ask Lemon?

@JefMari
Copy link
Contributor Author

JefMari commented Nov 26, 2015

Ok! 💯

@JefMari
Copy link
Contributor Author

JefMari commented Nov 26, 2015

I ask sir lemon about the proper design, and this is what he wants.

image

hovered

image

@srph
Copy link
Contributor

srph commented Nov 26, 2015

What about the button in the checkut?

@JefMari
Copy link
Contributor Author

JefMari commented Nov 26, 2015

Update!
image
hovered
image

@@ -53,7 +53,7 @@
border: 1px solid $brand-gray;
}

&.-primary { @include btn-variant($brand-sidebar-lt); }
&.-primary { @include btn-variant($brand-primary); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a huge misunderstanding. Last time, we talked about changing the primary button to $brand-sidebar-lt. Why the change again? :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops

@JefMari
Copy link
Contributor Author

JefMari commented Nov 27, 2015

Update!
image

Question: should it also applied in inverted colors?

@srph
Copy link
Contributor

srph commented Nov 27, 2015

No. Also, we might remove the inverted soon (but it has nothing to do with this).

@JefMari
Copy link
Contributor Author

JefMari commented Nov 27, 2015

Noted! Gonna merged this na

&.-inverted.-default { @include btn-variant-inverted($brand-gray); }
&.-inverted.-primary { @include btn-variant-inverted($brand-sidebar-lt); }
&.-inverted.-primary { @include btn-variant-inverted($brand-primary); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

@srph
Copy link
Contributor

srph commented Nov 28, 2015

What's with the new color change?

@JefMari
Copy link
Contributor Author

JefMari commented Nov 29, 2015

I also did change of color in inverted, so i change it back :D

image

@srph
Copy link
Contributor

srph commented Nov 30, 2015

OK. Like I said, I have no solid opinion about this. I'd suggest you ask sir Lemon again if you're out of ideas ahah. Thanks

JefMari added a commit that referenced this pull request Dec 5, 2015
@JefMari JefMari merged commit 55ea616 into master Dec 5, 2015
@JefMari JefMari deleted the enchantment/new-button branch December 5, 2015 13:51
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.

2 participants