Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(types): add bootstrap sizes for Form.Label as column #3731

Merged

Conversation

@dalevfenton
Copy link
Contributor

dalevfenton commented May 1, 2019

I'm not sure if just extending FormLabelProps with ColProps is the preferred way to solve this issue or if you would want something that actually enforces that the column size props are only valid when column is true.

I can make the typing stricter if that's preferred. It also seems like generally types aren't imported around and reused so if that's a problem I can copy them over completely from Col.d.ts


export interface FormLabelProps {
export interface FormLabelProps extends ColProps {

This comment has been minimized.

Copy link
@jquense

jquense May 2, 2019

Member

I wonder if the right way to do this is via a type Union, e.g. FormLabel | FormLabelWithColProps there column is true or false specifically (instead of boolean) to allow TS to differentiate

This comment has been minimized.

Copy link
@dalevfenton

dalevfenton May 2, 2019

Author Contributor

I started to do that, but didn't see that throughout the other typings so I wasn't sure if there was a preferred style. Let me write it up with the type Union.

Copy link
Member

jquense left a comment

can you also add a few tests in simple-test.tsx for this

dalevfenton added 2 commits May 2, 2019
@dalevfenton

This comment has been minimized.

Copy link
Contributor Author

dalevfenton commented May 2, 2019

one thing to note is that since FormLabelProps is now a type instead of an interface if anyone is currently using that to extend their own interface it will break their interface declaration

@taion
taion approved these changes May 2, 2019
@dalevfenton

This comment has been minimized.

Copy link
Contributor Author

dalevfenton commented May 6, 2019

@jquense I think this should be good to go now.

@taion
taion approved these changes May 6, 2019
@mxschmitt mxschmitt requested a review from jquense May 21, 2019
@jquense jquense changed the title fix #3481 - add bootstrap sizes for Form.Label as column fix(types): add bootstrap sizes for Form.Label as column May 21, 2019
@jquense jquense merged commit cdb7b6b into react-bootstrap:master May 21, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@jquense

This comment has been minimized.

Copy link
Member

jquense commented May 21, 2019

thanks!

@dalevfenton dalevfenton deleted the dalevfenton:bugfix/3481-form-label-as-column branch May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.