fix(Icon, SVGIconContainer): add generic type param to specify root element type #6285
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fix(Icon, SVGIconContainer): add generic type param to specify root elementBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
suppress prefer-arrow-callbackBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Merge remote-tracking branch 'origin/develop' into ad/icon-html-attributesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
|
This change makes the props interface table a lot less usable than it used to be: 
Before: 
I'll look into improving this... |
update docsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this pull request:
This fixes a regression in @blueprintjs/core v5.1.x where
<Icon>elements no longer accepted HTML attributes likedraggable={true}. Previously, these attributes were unsafely allowed to be forwarded to the root element of any<Icon>component, even whentagName={null}and those the root element was an<svg>element which should not accept them.After much iteration, I figured out a way to make
<Icon>and<SVGIconContainer>each have a generic type parameter (similar to<Select>). This turned out to be a lot harder to do for function components which useReact.forwardRef()than for class components. See my code comments for more info.I added documentation and test cases to demonstrate the new behavior. This may be a bit of overkill (we could be a little unsafe and just allow
React.HTMLAttributesto go through to<Icon>root elements), but it turned out to be an interesting exercise in exploring TypeScript generics & conditional types. I think it makes the public API a little more expressive.Reviewers should focus on:
No regressions in
<Icon>component props (these should be mostly covered by test cases).Screenshot
N/A