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

Buttons color Error #74

Closed
JMGama opened this issue Dec 4, 2018 · 15 comments · Fixed by #352
Closed

Buttons color Error #74

JMGama opened this issue Dec 4, 2018 · 15 comments · Fixed by #352
Assignees
Labels
enhancement New feature or request released waiting - contributor Waiting for the contributor to address some situations

Comments

@JMGama
Copy link

JMGama commented Dec 4, 2018

The color of the buttons stays the same when you click it then you need to click other place to restore the original button color.
It's a little hard to explain.

ezgif com-video-to-gif

@fleeting
Copy link
Contributor

fleeting commented Dec 4, 2018

Wouldn't this be the correct behavior as your focus is still on the button since the button doesn't do anything?

@JMGama
Copy link
Author

JMGama commented Dec 4, 2018

The button has a click function that ends and the button remains the same color, is that okay?

@jjspace
Copy link
Contributor

jjspace commented Dec 4, 2018

It's because of how the styling is applied. It's changing the color on :focus of the button. Focus persists even after a click is done until you focus/click on something else. We could take the styling off of focus and just have it change on :hover and :active and this wouldn't happen. I personally think this might be better but that could be open to discussion.

@fleeting
Copy link
Contributor

fleeting commented Dec 4, 2018

This is just because the hover and focus states use the same styles. For accessibility we should tweak the focus state to be different from the hover and normal state.

Ultimately what's happening is the inset style is from the active state, which happens when you click in. However, just like when you tab through the buttons with the keyboard, your focus is now on that button from the click and since focus and hover use the same styles - it just style looks the same.

We shouldn't remove focus but it's worth giving it it's own style.

@jjspace
Copy link
Contributor

jjspace commented Dec 4, 2018

+1 for thinking of tab focus, I'd forgotten about that. In that case I think it might be best to keep the hover and focus styling the same for consistency right?

@fleeting
Copy link
Contributor

fleeting commented Dec 4, 2018

At a minimum, they should be the same just so focus is set. I think we should come up with a new focus state, maybe remove the hover styles and add a decent outline. I'll do a pull request for some accessibility tweaks.

@abdallahalsamman
Copy link
Member

I've tried reproducing this on safari and chrome(both OSX) but couldn't reproduce it.
Which browser are you on?

@abdallahalsamman abdallahalsamman added the waiting - contributor Waiting for the contributor to address some situations label Dec 8, 2018
@fleeting
Copy link
Contributor

fleeting commented Dec 8, 2018

I get it in Chrome on OS X, it's just the focus styles. https://github.com/nostalgic-css/NES.css/blob/master/scss/elements/buttons.scss#L6 should ideally be split up so focus has it's own unique styles different than the hover style.

@jjspace
Copy link
Contributor

jjspace commented Dec 8, 2018

I can confirm that this happens in Chrome 70 on OSX and Chrome 70, FF 63 and Edge on Windows. However for some reason it does not seem like Safari and FF 63 keep focus state when you stop hovering on OSX so this problem does not exist there.
I think it might be worth separating the styles to handle them independently but with the fact that tab focus is essentially the mouse-less equivalent of hover I think they should stay the same style if not very similar. The "bug" just has to do with how browsers handle focus state and not really something we could get around while maintaining similar styles for both

@abdallahalsamman
Copy link
Member

I see.
@fleeting are you still going to open a PR for this issue? 🙈

@trezy trezy added enhancement New feature or request and removed improvement labels Dec 9, 2018
@youngkaneda
Copy link
Contributor

youngkaneda commented Apr 30, 2019

So what would be better, remove the effect in focus, or add a different focus effect? Since they are the same.

@BcRikko
Copy link
Member

BcRikko commented Jun 11, 2019

https://github.com/nostalgic-css/NES.css/blob/master/scss/elements/buttons.scss#L6 should ideally be split up so focus has it's own unique styles different than the hover style.

I agree 👍


I splitted the focus and hover styles. ✂️

focus

// buttons.scss
@mixin btn-style($color, $background, $hover-background, $shadow) {
  //...
  &:focus {
    box-shadow: 0 0 0 6px rgba($shadow, 0.3);
  }
  //...
}

@JMGama
Copy link
Author

JMGama commented Jun 11, 2019

I think the shadow of the focus looks a little weird, but it's great that now the styles are splitted. 👍

@youngkaneda
Copy link
Contributor

a simple and great solution

@BcRikko
Copy link
Member

BcRikko commented Sep 30, 2019

🎉 This issue has been resolved in version 2.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released waiting - contributor Waiting for the contributor to address some situations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants