moxystudio / next-with-moxy

MOXY's boilerplate to accelerate the setup of new Next.js based web applications
https://next-with.moxy.tech
MIT License
101 stars 11 forks source link

fix: remove outline from unstyled-button mixin #103

Closed jdiogopeixoto closed 3 years ago

jdiogopeixoto commented 4 years ago

We don't need to remove the outline as it is already handled by the react-keyboard-only-outlines package.

github-actions[bot] commented 4 years ago

This pull-request can be previewed at https://next-with-moxy-pr-103.now.sh. Please check the Zeit Now status below to see when it's ready.

codecov[bot] commented 4 years ago

Codecov Report

Merging #103 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #103   +/-   ##
=======================================
  Coverage   97.00%   97.00%           
=======================================
  Files          19       19           
  Lines         100      100           
  Branches        9        9           
=======================================
  Hits           97       97           
  Misses          3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update da8562a...c64a178. Read the comment docs.

satazor commented 4 years ago

@jdiogopeixoto I don't think this is entirely true. The react-keyboard-only-outlines only handles focus when using the keyboard. When you are using the mouse, it doesn't do anything.

jdiogopeixoto commented 4 years ago

@satazor By using react-keyboard-only-outlines we only have outlines when using a keyboard which is what we want, right?

Also the way we have it right now the mixin is removing the outlines when using a keyboard, which is not good for accessibility.

satazor commented 4 years ago

By using react-keyboard-only-outlines we only have outlines when using a keyboard which is what we want, right?

Correct.

Also the way we have it right now the mixin is removing the outlines when using a keyboard, which is not good for accessibility.

It depends on where this mixing is being used, but yes, generally you want outlines, even on unstyled buttons.

//cc @nlfonseca

PedroMiguelSS commented 4 years ago

It depends on where this mixing is being used, but yes, generally you want outlines, even on unstyled buttons.

Yap, you said it right. We want outlines even on unstyled buttons but just when you are using the keyboard. Usually, when I think of unstyled buttons I imagine them without the outlines. The only exception is the keyboard navigation as I said above ☝️

nlfonseca commented 4 years ago

As you can see here after we click (mouse interaction) the "unstyled button" it appears with an outline (in chrome). This is something that we don't want. The purpose of this mixin is to reset all styles of the button and after if we need something specific, like the outline for the keyboard we need to add that styles.

nlfonseca commented 4 years ago

but if you check with the react-keyboard-only-outlines package, and the above case is fixed, we can proceed with this PR.

jdiogopeixoto commented 4 years ago

As you can see here after we click (mouse interaction) the "unstyled button" it appears with an outline (in chrome). This is something that we don't want.

This doesn't happen because we are using react-keyboard-only-outlines and the package removes the outline when interacting with the mouse.

The purpose of this mixin is to reset all styles of the button and after if we need something specific, like the outline for the keyboard we need to add that styles.

I do agree to remove all the styles but the outline. We always want to have the outline for the keyboard navigation.

nlfonseca commented 4 years ago

I do agree to remove all the styles but the outline. We always want to have the outline for the keyboard navigation.

normally yes, but it's not necessarily true that we want always the outline. the keyboard navigation need's visual feedback? correct! but can be one outline or another style.

Anyway, we can proceed with this PR and for the specific case that I said above, we remove the outline directly on the button.

We will have always the two scenarios. I'm okay with the two.

satazor commented 3 years ago

Done in https://github.com/moxystudio/next-with-moxy/commit/ba2cafca7b41203d0737b2f5a1745eadd1f783a4

Thank you @jdiogopeixoto