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

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jquense left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dalevfenton
Copy link
Contributor Author

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

@dalevfenton
Copy link
Contributor Author

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

@mxschmitt mxschmitt requested a review from jquense May 21, 2019 20:03
@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
@jquense
Copy link
Member

jquense commented May 21, 2019

thanks!

@dalevfenton dalevfenton deleted the bugfix/3481-form-label-as-column branch May 21, 2019 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants