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 refactoring #365

Merged
merged 3 commits into from Jun 26, 2014
Merged

Button refactoring #365

merged 3 commits into from Jun 26, 2014

Conversation

klamping
Copy link
Contributor

BREAKING CHANGE: This change includes an overhaul of the button styles.
Removes the 'primary', 'button-blue', and 'button-green' styles

  • Default button color remains blue.
  • Added new 'button-positive' class for green button and 'button-negative' class for red button.
  • Moved all button styles into 'rxButton' directory for consistency

rxbutton
buttons

/cc @mocha @ericw

&:focus {
background: @buttonNegativeBgHover;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

are you missing a } somewhere in here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, had to read the full file

@halkon
Copy link
Contributor

halkon commented Jun 25, 2014

So if Travis lets you, then LGTM

Kevin Lamping added 3 commits June 25, 2014 17:44
BREAKING CHANGE: This change includes an overhaul of the button styles.
**Removes the 'primary', 'button-blue', and 'button-green' styles**
- Default button color remains blue.
- Added new 'button-positive' class for green button and 'button-negative' class for red button.
- Moved all button styles into 'rxButton' directory for consistency
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0184956 on 277-buttons into * on v1.0.0*.

@klamping
Copy link
Contributor Author

FINALLY PASSED!

klamping added a commit that referenced this pull request Jun 26, 2014
@klamping klamping merged commit 755582a into v1.0.0 Jun 26, 2014
@klamping klamping deleted the 277-buttons branch June 26, 2014 17:49
@klamping klamping mentioned this pull request Jun 27, 2014
@klamping klamping mentioned this pull request Jul 14, 2014
@@ -49,6 +55,7 @@
@tableCellText: #555;

@buttonText: @white;
@buttonDisabledText: #f0efef;

@orangeText: #ffa61b;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking: any context for orangeText? Could this be more semantic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used for the 'send feedback' link in rxApp, but it's hard to make this one "semantic" without tying it solely to that link. I have trouble with semantics and the vars.less file all together, since there is a lot of fuzziness to it all. Suggestions welcome!

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair concern, but it's a little jarring to go buttoncolor, bordercolor, ORANGE, you know? Maybe menuEmphasisText or some derivitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I get the same feeling here. Maybe lightEmphasisText or linkWithDarkBgText to point out that it's for a dark bg?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with the former, although the person that will use this the most is probably you, so something that won't help you forget*

  • Besides Orange ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slightyOrangeTextUsedForMakingTextOrange it is!

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