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 collapse.tsx for TS 2.4 #1296

Merged
merged 1 commit into from
Jun 28, 2017

Conversation

sandersn
Copy link
Contributor

React.HTMLAttributes requires overflowY to be a member of a string union: "initial" | "inherit" | "unset" | "auto" | "hidden" | "scroll" | "visible" | undefined. In Collapse.render, the overflowY needs to be given the explicit type "visible" | undefined in order to be assignable to HTMLAttributes. Otherwise, it gets the type string, which is too general.

Fixes #1237

See also DefinitelyTyped/DefinitelyTyped#17581, which this change depends on.
See also discussion at microsoft/TypeScript#16047.

React.HTMLAttributes requires `overflowY` to be a member of a string
union: `"initial" | "inherit" | "unset" | "auto" | "hidden" | "scroll" |
"visible" | undefined`. In Collapse.render, the `overflowY` needs to be
given the explicit type `"visible" | undefined` in order to be
assignable to HTMLAttributes. Otherwise, it gets the type `string`,
which is too general.
@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @sandersn! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@sandersn
Copy link
Contributor Author

Note that this change can be merged before DefinitelyTyped/DefinitelyTyped#17581, but it won't fix the build error with Typescript 2.4 until both are merged.

@cmslewis cmslewis requested a review from adidahiya June 28, 2017 19:27
@cmslewis
Copy link
Contributor

Thanks for the contribution, @sandersn! My personal preference is to wait for DefinitelyTyped/DefinitelyTyped#17581 to merge. @themadcreator or @gscshoyru: quick thoughts?

@sandersn
Copy link
Contributor Author

Actually, now that I think about it, it's probably better to merge this change first so that TS 2.3 users (and earlier) aren't broken when the React types change.

The reason is that with the current React types, the return type of Collapse.render isn't really checked in 2.3. When DefinitelyTyped/DefinitelyTyped#17581 goes in, overflowY will be checked better in both 2.3 and 2.4, and start failing.

@cmslewis
Copy link
Contributor

@sandersn Gotcha, that makes sense. Lemme circle up with one other remote Blueprint dev to make sure we're in sync (the other locals are all out today).

@cmslewis cmslewis requested review from themadcreator and gscshoyru and removed request for adidahiya June 28, 2017 19:50
@cmslewis cmslewis merged commit 504dd9a into palantir:master Jun 28, 2017
@llorca llorca mentioned this pull request Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants