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

docs(Button): add example with replicating button behaviour #1951

Merged
merged 1 commit into from
Aug 13, 2017

Conversation

layershifter
Copy link
Member

Fixes #1797.

@codecov-io
Copy link

codecov-io commented Aug 10, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1951   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files         148      148           
  Lines        2637     2637           
=======================================
  Hits         2631     2631           
  Misses          6        6
Impacted Files Coverage Δ
src/elements/Button/Button.js 100% <ø> (ø) ⬆️

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 69553ca...d37f9b0. Read the comment docs.

@levithomason
Copy link
Member

How about we just bake this into the button itself?

@layershifter
Copy link
Member Author

layershifter commented Aug 11, 2017

Yes, it was an initial idea, but how we will handle it? In case when Button will be button it will receive click event when key pressed, too. So we will have double click there.

@levithomason
Copy link
Member

Could we not check ElementType !== 'button' then add onKeyPress, tabIndex, and role? I'm imagining using the overrideProps in the factory to wrap the user's props for these.

I haven't tested any of this so I could be wrong, but it seems like we could bake in the same logic the user would manually add.

@layershifter
Copy link
Member Author

#1879 will disable ability to this check with ElementType !== 'button' because ElementType will return withRef HOC.

Possible we can handle this in event handler with e.target.tagName, if it will not be button, we call onClick. What you think?

@levithomason
Copy link
Member

Hmm, you are right. Actually, anything other than a DOM component will pose this problem. I think given the complication around trying to detect the type, we go with your current solution.

Thanks for the careful thought.

@levithomason levithomason merged commit 3c22d18 into master Aug 13, 2017
@levithomason levithomason deleted the fix/button-role branch August 13, 2017 16:54
@levithomason
Copy link
Member

Released in semantic-ui-react@0.71.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants