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

Continued styles and text tidying #892

Merged
merged 14 commits into from
Apr 5, 2017

Conversation

justinmcdavid
Copy link

I've edited strings for clarity/consistency in a number of places.

Updated primary/secondary-action buttons to be consistently styled.

Added hover states to a number of buttons that didn't previously have them.

Made some minor style changes (rule colors, heading spacing)

Removed underline effects on links, but maintained hover-color and cursor changes.

@millayr
Copy link
Contributor

millayr commented Mar 28, 2017

Looks like you're reintroducing the bug fixed in #891. You might want to consider rebasing this branch onto master.

color: @buttonText;
background-color: @primaryGreen;
border-color: @primaryGreen;
&:disabled, &[disabled]:hover {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor but my opinion is using pointer-events: none; instead of specifying &[disabled]:hover could be a bit cleaner.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

color: @buttonText;
background-color: @secondaryBlue;
border-color: @secondaryBlue;
&:disabled, &[disabled] {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit redundant without the :hover. Also consider my suggestion in the previous comment.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

color: @secondaryBlue;
background-color: @buttonText;
border: 2px solid @secondaryBlue;
&:disabled, &[disabled] {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@@ -177,13 +177,12 @@
font-weight: normal;
font-family: helvetica;
.box-sizing(border-box);
background-color: #564E4C;
background-color: @brandDark1
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a semi colon

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@millayr
Copy link
Contributor

millayr commented Mar 29, 2017

Personal opinion is it'd be nice to have some kind of hover state change (other than a change of cursor) for the database names on the _all_dbs page.

color: @buttonText;
background-color: @dangerRed;
border-color: @dangerRed;
&:disabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to consider hover state here?

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@garrensmith garrensmith merged commit 1e548f1 into apache:master Apr 5, 2017
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

3 participants