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 edge-case refinements #84

Closed
wants to merge 13 commits into from
Closed

Button edge-case refinements #84

wants to merge 13 commits into from

Conversation

aaronshekey
Copy link

A number of button changes here, mostly focused on little edge-case polish.

Changes on the right.

  1. Focused states are important. I think we should maintain the focused border regardless of hover states.
    focus
  2. Selected buttons should still have a hover state since you can still interact with them.
    selected-hover
  3. Primary button removes text shadow in favor of a darker color for improved readability.
    primary
  4. Green is slightly darker site-wide. Improves readability by removing some of the fluorescence.
    context
  5. Primary button focus states have been adjusted slightly to account for the darker green.
    primary-focus
  6. Focused buttons within the button groups are now the highest in the z-order as opposed to being overlapped by the button to the right.
    focus-z

@mdo
Copy link
Contributor

mdo commented Mar 30, 2015

Not stoked about the last two changes. I'm in favor of darkening the text shadow, but not reversing the direction. The text color change on the disabled state also feels off—maybe a little muddy?

@aaronshekey
Copy link
Author

I can revert the shadow direction, but did so to match the light source of all the gradients throughout Primer. Do you just not like it visually or is there some rationale to having the shadow cast opposite the light source?

Darkened the disabled primary button. Is this any better or should I drop it?
context

@mdo
Copy link
Contributor

mdo commented Mar 30, 2015

The intent of the text-shadow is to illicit an embossed effect of sorts. The text isn't sitting on top of the button, it's inset. Still not a fan of the disabled primary button change :).

@aaronshekey
Copy link
Author

@mdo Okie dokie. I've reverted those last two points and updated the screengrabs. I disagree that they should be embossed, but hey, it's your huge mistake, not mine 😉

@mdo
Copy link
Contributor

mdo commented Mar 30, 2015

Originally I had no text-shadow on them, but that looked off a bit. I wouldn't be opposed to seeing the colors change so we can nuke the text-shadow entirely though—we don't have one on any of our other buttons I think.

Aaron Shekey added 3 commits March 31, 2015 09:44
Kills text shadow and slightly darkens green
Darkens up the disabled state just a little to eliminate the need for a
text-shadow
Darkens the green color used throughout to provide a bit more contrast
and reduce fluorescence.
@aaronshekey
Copy link
Author

I wouldn't be opposed to seeing the colors change

Try these new colors out. I think it improves readability without losing the soul of the original green. It only loses a little fluorescence is all.

Aaron Shekey added 3 commits March 31, 2015 12:27
With the darker color, we need to make sure the blue outline isn't
vibrating too much.
Focused objects should stay on top of everything in the z-order stack.
@jasonlong
Copy link
Contributor

Most of these are 👌 by me.

I'm not sure how I feel about darkening the primary buttons though. Most of our brand colors are pretty vibrant and this one feels pretty heavy.

screen shot 2015-03-31 at 2 05 51 pm

@bleikamp
Copy link

Definitely like most of these changes.

I agree with @jasonlong's point, and I think my only big hang up would be the changes to the primary button color. Maybe it's something we'd use for a few days and wouldn't notice again, but it definitely feels a little out of place in the screenshots.

I would be open to adjusting our colors at some point, as I think the green, red, and orange in particular are pretty bright. But maybe that's a conversation for another PR? It seems like we could get this merged pretty quickly if we moved that conversation to another PR.

@fabianperez
Copy link

👎 on the green. Everything else looks good.

@aaronshekey
Copy link
Author

Perhaps if we flatten the green buttons by getting rid of the top gradient highlight. I love the green as a large swatch, but when it's applied to thin type and buttons with white text, it really lacks contrast.

I think this is a really good example of that.
all-is-well

I think the other stuff is shippable. I just worry that the focus state for buttons might get a little obnoxious in practice. I'll sit with it for a bit.

@bleikamp
Copy link

Perhaps if we flatten the green buttons by getting rid of the top gradient highlight

I'd like to avoid going totally flat, but I perhaps a more subtle gradient? Affordance still seems important, and one flat button seems a little off, too.

@jasonlong
Copy link
Contributor

Agreed that we should tackle any brand color and UI element tweaks separately. I think we can definitely make some improvements, but there are a lot of use cases to consider.

@aaronshekey aaronshekey mentioned this pull request Mar 31, 2015
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.

5 participants