Conversation
bmdalex
requested review from
dvdzkwsk,
kuzhelov and
levithomason
as code owners
bmdalex
force-pushed
the
feat/button-fluid
branch
from
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.
TS uses public by default in classes. Let's keep the code concise.
There was a problem hiding this comment.
@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.
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.
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.
This should already be typed by extension of the UIComponent, no?
There was a problem hiding this comment.
doesn't look like it...
| root: ({ props, variables }: { props: IButtonProps; variables: IButtonVariables }) => { | ||
| const { children, circular, content, fluid, type } = props | ||
| const primary = type === 'primary' | ||
| const secondary = type === 'secondary' |
3 participants
Button (
fluidprop)This PR will introduce possibility to specify
fluidbuttonsTODO
API Proposal
fluid
fluidwill 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
shapeprop.]