-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
0e082ec
to
72a2970
Compare
@levithomason |
Codecov Report
@@ Coverage Diff @@
## master #6 +/- ##
==========================================
+ Coverage 84.08% 84.22% +0.14%
==========================================
Files 58 58
Lines 779 786 +7
Branches 157 158 +1
==========================================
+ Hits 655 662 +7
Misses 120 120
Partials 4 4
Continue to review full report at Codecov.
|
class Button extends UIComponent<any, any> { | ||
static displayName = 'Button' | ||
class Button extends UIComponent<IButtonProps, any> { | ||
public static displayName = 'Button' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS uses public by default in classes. Let's keep the code concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levithomason - I'm aware, but I don't like that feature; I feel that with public by default
developers are prone to exposing methods that should be private just because the habit of adding a modifier is not enforced; I don't mind reverting tho'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Want to make sure it is addressed properly as well. If you feel very strongly for this topic, please raise an RFC and let's have some good conversation around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other benefit of explicitly deciding we want to declare public is that for an open source project it's going to be easier for contributors to pick up on what should or should not be public. Not sure if there is a tslint rule for this actually, which would be great.
I am still new to TS and I was not aware of the default public nature. Always declaring public/private is probably going to help new contributors.
ElementType, | ||
classes, | ||
rest, | ||
}: IRenderResultConfig<IButtonProps>): ReactNode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should already be typed by extension of the UIComponent, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
root: ({ props, variables }: { props: IButtonProps; variables: IButtonVariables }) => { | ||
const { children, circular, content, fluid, type } = props | ||
const primary = type === 'primary' | ||
const secondary = type === 'secondary' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Button (
fluid
prop)This PR will introduce possibility to specify
fluid
buttonsTODO
API Proposal
fluid
fluid
will allow a button to take the width of its container.[The circular prop is one of many shapes. Proposing shapes be grouped as enums under the
shape
prop.]