Create button component (duplicate)

dotkom / design-system

Component library, maybe

https://design.online.ntnu.no

MIT License

4 stars 4 forks source link

Create button component (duplicate) #103

Closed niklasmh closed 4 years ago

niklasmh commented 4 years ago

This was supposed to fix #25 thus seems like it is a duplicate PR now. I will try to merge this with PR #102 as the PRs have the same idea.

Current status:

plusk commented 4 years ago

Not big into the third category of buttons, it's very material-ripoff. The other buttons look pretty solid, and I like that you made the outline-mode for all the different kinds of buttons. The disabled buttons doesn't look disabled at all though. I can look into merging the two PRs

niklasmh commented 4 years ago

I was in a rush when posting the picture so the disabled button should not look like that at all. Will update the image now.

You caught me redhanded, I have been highly inspired by https://material-ui.com/components/buttons/#text-buttons. I can add an underline to the third button category if that makes it less ripoff:

niklasmh commented 4 years ago

Just continue merging. I have changed:

variation -> variant (just used the name variant from material-ui)
inverted -> outline (you used outline with was way more descriptive)
text -> value (using the default HTML attr value while also enabling children to be used)
added your part as a comment as the merge is in progress. I like the way you used the css to define different styles for each variant. Just do that, it looks much cleaner.

plusk commented 4 years ago

I made some changes, mostly trying to simplify. I removed the "flat" variant because I don't see the usecase. We've never had a button like that before, but it can be added again if there's ever a usecase for it. Taking in children is a bit "risky" because really we just want to take in a short string. We'll look at icons when we get to icons, no need to speculate on how they'll work for now. It looks good enough for now, so I'll merge when it passes CI.