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

Include focus styling on button #1648

Closed
ngbrown opened this issue Oct 7, 2019 · 5 comments · Fixed by #1660 · May be fixed by largerock/largerock.com#5
Closed

Include focus styling on button #1648

ngbrown opened this issue Oct 7, 2019 · 5 comments · Fixed by #1660 · May be fixed by largerock/largerock.com#5

Comments

@ngbrown
Copy link

ngbrown commented Oct 7, 2019

  • components: button
  • reactstrap version 8.0.1

The bootstrap 4 js adds focus class to the styling of buttons on the focus event and removes it on the blur event. This helps for keyboard navigation of buttons.

https://github.com/twbs/bootstrap/blob/3d12b541c488ea09efced2fb987fcbf384c656bb/js/src/button.js#L164-L178

Reactstrap should do the same.

@TheSharpieOne
Copy link
Member

TheSharpieOne commented Oct 8, 2019

I am not 100% sure why they do this, but it is not for every button. It is only for buttons which match data-toggle^="button" (the data-toggle attribute starts with "button")
It seems to only exist within the Button Plugin section of the button docs.
From what I can tell, it for when the "button" (the element with .btn) is not a <button> / focusable item (such as when it's a label).
We probably need to create a new component (I'm not sure if it would fit into the current button component), some kind of state/toggle button component.
IIRC, toggling button active class was left to the user since it required state and most stateful things were left to the user of the lib. I think now, that this is needed and we should add it to the lib.

@Ashniu123
Copy link
Contributor

I think I can do this, pls assign it to me

@TheSharpieOne
Copy link
Member

It's all your @Ashniu123

@Ashniu123
Copy link
Contributor

Ashniu123 commented Oct 10, 2019

Thanks!
This is what I'll do:

  • Make a new component - ButtonToggle (or whichever name is more suitable)
  • Replicate Button component
  • Add blur and focus event handlers for removing and adding the .focus class respectively

Sounds good?

@TheSharpieOne
Copy link
Member

I would try to import and use the existing Button component if possible instead of replicating it. Basically manage the state of focus to conditionally apply className="focus to the existing Button.

Ashniu123 added a commit to Ashniu123/reactstrap that referenced this issue Oct 12, 2019
TheSharpieOne pushed a commit that referenced this issue Oct 28, 2019
* feat(ButtonToggle): add focus styling on button and toggle

resolves issue #1648

* refactor(ButtonToggle): user can define other actions without interfering with component

execute user defined onClick,onFocus,onBlur functions before its own

* refactor(ButtonToggle): no toggle if button is disabled

the user defined onClick function will still execute though, as intended

* refactor(ButtonToggle): add missing event param for onClick

* refactor(ButtonToggle): call user's onClick, onFocus, onBlur if it exists

Add defaultProps to pass default values

* refactor(ButtonToggle): rename default value for toggle. add file to index.js

* test(ButtonToggle): add test cases for component

* Update ButtonToggle.js

* Update ButtonToggle.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment