clarisights / KnitUI

A React Component library implementing the Knit design language
https://knitui.design
16 stars 12 forks source link

Button: Enhancements, cleanup, match design spec #159

Closed anshumanv closed 4 years ago

netlify[bot] commented 4 years ago

Deploy preview for knit-ui ready!

Built with commit acbcc351f4548dd2cd12c3fa9c9b99298351fbaa

https://deploy-preview-159--knit-ui.netlify.app

arorayash commented 4 years ago

Functionality wise, looks good. However, the README is pretty vague at places. Can you please clarify the role and type of props. Eg, insetPosition doesn't say anything about the values the comp expects, is it -1, 0, 1 or "left", "right" or "LEFT", "RIGHT"? Similar case with style, some places we are using SC css props(which expects string), where as here it looks like JS object is expected. Can we use one pattern for this?

@prasook-jain @anshumanv

anshumanv commented 4 years ago

However, the README is pretty vague at places. Can you please clarify the role and type of props. Eg, insetPosition doesn't say anything about the values the comp expects, is it -1, 0, 1 or "left", "right" or "LEFT", "RIGHT"?

Will add possible props in the table.

Similar case with style, some places we are using SC css props(which expects string), where as here it looks like JS object is expected. Can we use one pattern for this?

In favour of JS object, style as string ❌

prasook-jain commented 4 years ago

Similar case with style, some places we are using SC css props(which expects string), where as here it looks like JS object is expected. Can we use one pattern for this?

It's not upto library, we should allowing both style & css using {...rest}. preference should be of user.

Also css props is better option, which have more reach, we can add :focus, :active, or reach down to its child. It's basically like styling like styledComponent. @arorayash @anshumanv

arorayash commented 4 years ago

Similar case with style, some places we are using SC css props(which expects string), where as here it looks like JS object is expected. Can we use one pattern for this?

It's not upto library, we should allowing both style & css using {...rest}. preference should be of user.

Also css props is better option, which have more reach, we can add :focus, :active, or reach down to its child. It's basically like styling like styledComponent. @arorayash @anshumanv

Right now, we are the only users of the library. So if we support one format, it's good enough. Our focus should be only on our use cases in the current scenario.

And I totally agree with keeping the string format.

@anshumanv @prasook-jain

arorayash commented 4 years ago

Can you update the component to support string type for styles? @anshumanv

anshumanv commented 4 years ago

Can you update the component to support string type for styles? @anshumanv

Will update today. 👍

arorayash commented 4 years ago

Any updates on this? @anshumanv

anshumanv commented 4 years ago

Forgot about it, updates coming in a bit. 👍

anshumanv commented 4 years ago

Updated @arorayash @prasook-jain, PTAL. ✨

Fixing tests meanwhile.

anshumanv commented 4 years ago

@prasook-jain please run a check about potential code cleanup which I may have missed if any.