fibo / trunx

Super Saiyan React components, son of awesome Bulma
MIT License
64 stars 14 forks source link

Button render as <a> when onClick attribute is defined #21

Closed jekhoxy closed 5 years ago

jekhoxy commented 5 years ago

Hello @fibo

I have an issue with the Button render. As it said in the docs, if a href prop is given, Button is render as a link. But in my case, I don't have a href prop, only onClick and disabled props and it render as link anyway.

If I remove the onClick prop, render as button works.

Here my snippet: <Button onClick={this.myHandle} disabled={condition}> MyButton </button>

Thanks to take a look.

cordially

jekho

ashnur commented 5 years ago

It's href OR onClick, see here https://github.com/fibo/trunx/blob/v0.23.0/component/Button.js#L67

ashnur commented 5 years ago

I think that AnchorButton and ButtonButton should be two different components, I don't see why we need to overload an interface this much? :)

jekhoxy commented 5 years ago

Ok I see. Actually, I just would like to disable a simple button for some condition. And as a button, it's normal to have a onClick prop. So it's strange I could not use both of them like this.

ashnur commented 5 years ago

Just copy the source of the <Button> locally and rewrite it until @fibo shows up and fixes it ;)

fibo commented 5 years ago

Hi thank you for reporting it. I am in Thessaloniki now, I was invited as a speaker to DEVit conference, it was really great!

So today I will go back in Italy. I think I can have a look in the next days.

fibo commented 5 years ago

@jekhoxy thank you very much for your feedback, you are completely right. Now it should be rendered as an anchor only if there is an href prop https://github.com/fibo/trunx/commit/f55288e680fef460d1a197a182246f769ccd0d38

@ashnur this behaviour tries to be fair with Bulma interface, I found many snippets on Bulma documentation website using something like

<a class="button">my button</a>

and th best way I found to replicate this is using a Button component. However I think we are also moving to that direction, in fact I am creating components like A, Li hoping this can improve coding expressiveness. Here the logic is: if there is a Bulma class with a tag name, create a component, for example

Bulma class: button ----> Trunx Component: Button

In other cases we have Bulma classes that enrich a tag, for example

<li class="is-active">foo</li>

hence there is a Li component that implements that, for example

<Li isActive>foo</Li>

@jekhoxy please try again with Trunx version v0.23.2 and let me know if there are still issues.

fibo commented 5 years ago

@ashnur LOL in React MDC a biG company created a Button implementation really similar to Trunx button, so we are probably on the right path: https://github.com/material-components/material-components-web-react/blob/master/packages/button/index.tsx

ashnur commented 5 years ago

@fibo I think you are falsely assuming that they actually know better than we do :). But why would that be the case? Do they possess arcane knowledge that gives them access to some higher truth than our own experience?

On the other hand, I was 100% joking with the idea AnchorButton and ButtonButton, I thought it was obvious from the names and the smiley.

My point is that there is a three way many-to-many relationship between html tags, frontend components and css classes. And this doesn't mean A*B*C but actually (A+B+C)^3 === A^3 + 3 A^2 B + 3 A B^2 + B^3 + 3 A^2 C + 6 A B C + 3 B^2 C + 3 A C^2 + 3 B C^2 + C^3 Which means that I would rather keep these separate and only combine them in practice. Wouldn't ever decide ahead of time of an anchor if it's a button or not. That is, I wouldn't write code that is specific to any one scenario, unless there is something to be optimized for.

jekhoxy commented 5 years ago

@fibo I applied your changes into Button.js in my repo, I updated to 0.23.2 but I don't see your changes.

Anyway, it works as I would.

Great job, thanks ;)

fibo commented 5 years ago

@jekhoxy oppsss yes, I published but not built. Now version 0.24 should work.

fibo commented 5 years ago

@ashnur at Thessaloniki I knew this girl that is working at San Francisco in the Material team, in fact she talked about their pattern at the conference.

She knows better then us, she can dance better: https://www.instagram.com/bonniezzzhou/

ashnur commented 5 years ago

But I am not competing with her dancing skills, right? :) Large companies have a different view-point that defines a completely separated perspective than what others do. They have hundreds of developers to churn out code and they can brute-force solve issues that others would spend years on. I rather not try to compete with that either. So appeal to authority of any form I think is a misguided way to argue since there is no authority in these questions higher than the actual people actually working with the code. And that's you I think, and anyone who uses the library after you (like @jekhoxy for example ;) . Following what others' say (be it me or Google or the UNICEF) will not result in actual problems solved only solutions to non-problems imported.

ashnur commented 5 years ago

Oh, and this blatant adherence to "influencers'" views is the first for me, and the more I think about it the less funny the joke is.

fibo commented 5 years ago

No adherence to influencers views, Trunx is an indipendent project. MDC architecture is different, and I also found things that I don't like or I would do better (for example classes are not typed as in Trunx) but I always like to dig into the code of others and explore a little bit.