coinbase / onchainkit

React components and TypeScript utilities to help you build top-tier onchain apps.
https://onchainkit.xyz
MIT License
641 stars 167 forks source link

Feature Request: Unstyled Buttons #1116

Open mykcryptodev opened 3 months ago

mykcryptodev commented 3 months ago

Describe the solution you'd like

Allow me to pass a prop to a button like TransactionButton that removes all styling so that I can be sure that my classNames are the only styles applied.

Describe alternatives you've considered.

I've tried fiddling with classNames and using !important to try to get my custom styles to be applied as desired but it would be much easier if I could tell onchainkit to be styleless

0xGurmy commented 3 months ago

I was able to avoid using !important to fully style the button by targeting the span inside the buttons, but this took me a bit of playing around before it became evident that I needed to target the span within the wallet button.

I don't know if it's possible, but it would be really nice if I could style everything with the button class, without the span, because I have my buttons styled in my MaterialUI theme.

Adding here because I don't know if this deserves it's own issue 🫠

Zizzamia commented 3 months ago

@mykcryptodev and @0xGurmy can you share code example of your experience. I have a few ideas on how to solve this, but i would like to get more info on your experience first.

mykcryptodev commented 3 months ago

@Zizzamia here is an example that I whipped up

Here are two buttons where I provide the same style (btn btn-primary). My

Zizzamia commented 2 months ago

@mykcryptodev so to give more control on this particular case, we are going to explore a new component added to the Wallet family, <ConnectWalletText>.

Here the PR https://github.com/coinbase/onchainkit/pull/1222

You are going to use it by doing

<ConnectWallet>
  <ConnectWalletText>Connect Wallet 🌊</ConnectWalletText>
  <Avatar address={address} className="h-6 w-6" />
  <Name />
</ConnectWallet>

This is not a perfect solution, but for sure is a step forward to the text prop that was not allowing you custom style.

More I think about this, and more I am realizing that the text prop in general is a bad idea, and whenever we use it we should remove it.

For now we are not deprecating the text prop yet, but it's something in coming releases we will. Probably in 5~6 weeks, so we give time to the community to adjust to using <ConnectWalletText>.

Oh, and I am going to close this Issue after we land this in the next version and I will have updated the docs.

mykcryptodev commented 2 months ago

Understood. thanks for considering this! So it sounds like the prevailing idea is to give the developer control over each nested element within an onchainkit component.

In this example, the span element that styles the connect button text can be styled by including the component and applying classes there.

It sounds like if this pattern continues throughout onchainkit, there would be a world where if I wanted to style the text of a , there would be a component? Is that the idea?

Zizzamia commented 2 months ago

Yeah, at least for now seems the most balanced way.

But, it's fair to stay vigilant on this design decision and keep re-evaluate that overtime.

mykcryptodev commented 2 months ago

Here is different example that falls into the same bucket where onchainkit's styling throws me off

Here I want to have my onchainkit transaction button sitting next to a different button that is not from onchain kit. I use flex to put them next to eachother and I use items-center to vertically align them however you'll notice that my onchainkit button is not aligned Screenshot 2024-09-13 at 9 32 57 AM

this is due to onchainkit adding margin above the button in it's own styling Screenshot 2024-09-13 at 9 35 03 AM

Unfortunately, I cannot simply pass in a tailwind class to remove the top margin on the transaction button because onchainkit's margin is on the button element within the div that I am applying classes on Screenshot 2024-09-13 at 9 35 45 AM

I would need to write my own custom class to handle this situation which is not ideal

Zizzamia commented 2 months ago

@mykcryptodev great call out, but here 2 points:

Point 1 You can control directly the style with <TransactionButton />, and do

<TransactionButton className="mt-0" />

and it will work, no? That's the all point of having sub components you can directly control.

Point 2 Btw, a general point about margin, that I just realized, is that for any of the primary component we should allow margin style within OnchainKit. That's because primary components should not modify the space around them, but only within them. Anyway removed the mt-4 with this PR. https://github.com/coinbase/onchainkit/pull/1258

mykcryptodev commented 2 months ago

You can control directly the style with

@Zizzamia that does work. I have to get used to the pattern of having sub-components. Thanks for the tip

primary components should not modify the space around them

Agreed. Thanks for that update!