primitives icon indicating copy to clipboard operation
primitives copied to clipboard

[New Primitive] `Button`

Open asherccohen opened this issue 3 years ago • 29 comments

Feature request

Following a conversation with your maintainers I was suggested to create a reproducible demo of what I had in mind for a Radix Button component.

Overview

Buttons are simple elements at first, but can be tricky to implement considering all the different states they could be in. Also it's nice to build into buttons elements like start/end/loading icons.

Someone would also use them as links.

Examples in other libraries

Here's the link to my sandbox: https://codesandbox.io/s/silly-architecture-wuxrs?file=/src/index.tsx

Who does this impact? Who is this for?

Component libraries normally share primitive buttons, and Radix could benefit from one that has the proper semantic and use cases.

Additional context

For reference I have taken inspiration from BaseWeb's button. I use react-aria to simplify accessibility implementation (not my domain of knowledge), so I'll leave this to you guys to refine. I use styled-components and styled-system instead of stitches in my demo, just a shortcut as I haven't migrated to stiches yet (waiting to see if you take onboard the suggestion to allow tagged literals instead of style objects)

asherccohen avatar Oct 07 '21 08:10 asherccohen

Hey there, I think I may be missing some context, but what's the issue you're having? Thanks!

peduarte avatar Oct 07 '21 08:10 peduarte

Hey @asherccohen, thanks for adding this. We have had a few requests for this so I know that we have it on our todo list. I'm not entirely sure on a timeline but we'll pick this up 👍

jjenzz avatar Oct 07 '21 12:10 jjenzz

Hey there, I think I may be missing some context, but what's the issue you're having? Thanks!

Hi @peduarte, I had a chat on twitter and discord with Colm about introducing a Button component. I had created this for my own use, so I shared a codesandbox with you to refine the idea.

asherccohen avatar Oct 07 '21 12:10 asherccohen

Ah gotcha. This is a new component request. Thanks for clarifying. We'll prob spec it over the next few weeks.

peduarte avatar Oct 07 '21 14:10 peduarte

I suggest looking at what react-aria does in their useButton hook. Their way to compute isPressed for "better" active styles is pretty cool.

DaniGuardiola avatar Oct 07 '21 16:10 DaniGuardiola

@DaniGuardiola that is exactly what I used in my codesandbox.

Have a look.

I even show a dev warning to deprecate onClick and favour onPress.

asherccohen avatar Oct 07 '21 17:10 asherccohen

@asherccohen my bad, I didn't look at the codesandbox. Looks nice! FYI, react-aria will already show a warning for that. And you might want to research what happens when you try to use this as a trigger for a different primitive, for more context see #895 and this old discussion: https://github.com/radix-ui/primitives/discussions/822

Honestly, if Radix gives me a button like react-aria's that's compatible with other primitives out of the box, I will be super happy.

DaniGuardiola avatar Oct 07 '21 17:10 DaniGuardiola

Thanks for the comments, and I agree, a modern button out-of-the-box would be a nice to have.

As you mention I have had issues showing the button with the (now deprecated) as prop, for example to render it as a link. All styles get wiped out and I didn't have the time to investigate.

Luckily radix has moved to the asChild API.

I haven't refactored my implementation so I can't say if it fixes it.

asherccohen avatar Oct 07 '21 17:10 asherccohen

@asherccohen @DaniGuardiola I use React Interactive for buttons and links, as well as with the Radix primitives, to improve interactive styles on touch devices (touchActive state and eliminates the sticky hover bug), and keyboard navigation (focusFromKey state). It might work well for your use case too.

rafgraph avatar Oct 10 '21 16:10 rafgraph

As you mention I have had issues showing the button with the (now deprecated) as prop, for example to render it as a link. All styles get wiped out and I didn't have the time to investigate.

@asherccohen Sounds like this issue perhaps? https://github.com/modulz/stitches/issues/448

If you're not using stitches, most CSS-in-JS libraries have the same issue. Styled Components has a forwardedAs prop to solve it and Emotion has a shouldForwardProp API I believe. If it's something else entirely then, fingers crossed the asChild API solves it for you 🤞 let us know if not.

if Radix gives me a button like react-aria's

@DaniGuardiola What features from the react-aria button are you hoping for? The original feature request seems only to expect loading states so would be helpful to have a more exhaustive list 🙂

jjenzz avatar Oct 11 '21 12:10 jjenzz

Hi @jjenzz thanks for asking. We would find the loading states useful, but the deal-breaker for us are the more "native-like" hover/press styles, which react-aria exposes as the isPressed boolean it returns from usePress and useButton (which uses the former under the hood). It is also pretty nice to be able to just use the press events they expose and know it's gonna work across platforms and input methods.

I think this series of blog posts is relevant: https://react-spectrum.adobe.com/blog/building-a-button-part-1.html (you can find links to the next articles in the bottom of each)

It explain what they did and why they did it. I guess the concern about the events would be mostly solved by the pointer events API, but as they explain, it doesn't seem to be consistent enough yet. Maybe Radix could fix the behavior without creating new handlers (as they do with the "press events"), and just by adding additional logic to the existing point event handler props? That would prevent a lot of compatibility issues.

DaniGuardiola avatar Oct 11 '21 17:10 DaniGuardiola

@rafgraph thanks for sharing, this could be super helpful for us, at least until Radix introduces a button component.

DaniGuardiola avatar Oct 11 '21 17:10 DaniGuardiola

Thanks for this, I'll take a look.

Maybe Radix could fix the behavior without creating new handlers (as they do with the "press events"), and just by adding additional logic to the existing point event handler props? That would prevent a lot of compatibility issues.

This is definitely more along the lines of what we'd be likely to try.

jjenzz avatar Oct 11 '21 17:10 jjenzz

Interesting discussion 😄 Before starting to use Stitches + Radix I've always wondered "why isn't there a button in Radix?". I eventually built a Button component myself, it's probably a bit naive and can be improved but in my case the main functionality I needed, other than variants provided by Stitches, was the loading state. https://gist.github.com/mtt87/43d7d92974064b50b840a5a20b770840

I think this series of blog posts is relevant: https://react-spectrum.adobe.com/blog/building-a-button-part-1.html (you can find links to the next articles in the bottom of each)

Thanks for sharing this is useful 😄

mtt87 avatar Oct 15 '21 09:10 mtt87

Will this also include button group component if it affects accessibility?

joseDaKing avatar Feb 28 '22 17:02 joseDaKing

Potentially yes, as there may be things like roving focus to include in a group.

benoitgrelard avatar Mar 30 '22 12:03 benoitgrelard

Is this still something that may happen via the Radix team? Would be nice to not have to create a button that deals with all the states possible across touch devices, mouse driven devices and keyboard nav devices... not to mention screen readers etc....

thtliife avatar Feb 06 '23 05:02 thtliife

@benoitgrelard Would you be open for contributions here? Im using radix with adobe-aria (usePress) and things get hairy when mixing things like

<Tooltip.Trigger asChild>
  <Button /> // button using usePress
</Tooltip.Trigger>

Having a Button primitive controlling everything from radix would be ideal world

ivanbanov avatar Apr 18 '23 11:04 ivanbanov

@ivanbanov the problem is that usePress stops the propagation of the event and is not compatible with the web platform event APIs. The only sane solution is to not use it. Or, convince the React Spectrum team to rework it :P

My suggestion would be to just stop using it and go with vanilla HTML <button />. If you want the "improved hover" style, I wrote an alternative solution for it here: https://github.com/ariakit/ariakit/issues/1084#issuecomment-1230967579

DaniGuardiola avatar Apr 18 '23 19:04 DaniGuardiola

Any updates on this?

maxall41 avatar Jul 15 '23 22:07 maxall41

Hi, any updates on this? Thanks!

mushkin-v avatar Jul 31 '23 14:07 mushkin-v

For adobe-aria, there was a PR to enable continuePropagation that may fix some of the issues

https://github.com/adobe/react-spectrum/pull/4683

ivanbanov avatar Aug 01 '23 07:08 ivanbanov

Just happened across this issue. I made simple example of a React Aria button working with a Radix tooltip as in your example above and it seemed to work ok. You just have to make sure to send through the DOM props to the element in addition to the props from useButton. Was there something specific that wasn't working for you? https://codesandbox.io/s/empty-hill-sp754f?file=/src/App.js

devongovett avatar Aug 09 '23 02:08 devongovett

@devongovett onPointerDown never bubbles and radix needs it to close the DropdownMenu for example

https://codesandbox.io/s/radix-with-adobe-fail-gcv5nf?file=/src/App.js

ivanbanov avatar Aug 09 '23 06:08 ivanbanov

Thanks for the example! I did some investigation. I think this is not actually due to event propagation, but due to preventDefault(). usePress prevents default in order to work around browser issues and normalize focus management. It looks like Radix is checking the defaultPrevented property and decides not to handle the event if it is true: https://github.com/radix-ui/primitives/blob/3e0642e40038386d58da9fb1d812c2fbfe9f67c1/packages/core/primitive/src/primitive.tsx#L9

I'm not sure why Radix cares whether the browser default is prevented or not, but looks like you can solve the problem by making the Radix handlers run before the React Aria ones. Just swap the order of the mergeProps. Here's a working version: https://codesandbox.io/s/radix-usepress-3dhzmq?file=/src/App.js

devongovett avatar Aug 09 '23 18:08 devongovett

@devongovett the codesandbox link provided doesn't seem to be working (fyi)

jd-carroll avatar Aug 09 '23 18:08 jd-carroll

Oh sorry, I guess they changed the default to private. Should be fixed now.

devongovett avatar Aug 09 '23 18:08 devongovett

Is there a rationale for not including a Button component? Didn't see any answers here.

christoph-codes avatar Nov 28 '23 21:11 christoph-codes