Update Icons to use React.forwardRef & Remove support for Octicon
#943
Conversation
🦋 Changeset detectedLatest commit: ac2dc3f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
broccolinisoup
changed the title
createIconComponent utility func to return React.forwardRef createIconComponent utility func to return React.forwardRef
broccolinisoup
force-pushed
the
forwardRef-octicon
branch
from
f9b3a77
to
e88a092
Compare
lib/octicons_react/src/index.js
Outdated
| @@ -6,7 +6,7 @@ export default function Octicon({icon: Icon, children, ...props}) { | |||
| // eslint-disable-next-line github/unescaped-html-literal | |||
| '<Octicon> is deprecated. Use icon components on their own instead (e.g. <Octicon icon={AlertIcon} /> → <AlertIcon />)' | |||
| ) | |||
| return typeof Icon === 'function' ? <Icon {...props} /> : React.cloneElement(React.Children.only(children), props) | |||
| return typeof Icon === 'object' ? <Icon {...props} /> : React.cloneElement(React.Children.only(children), props) | |||
There was a problem hiding this comment.
I am a little unsure about this change 😵💫 Would there be any impact on consumer applications? 🤔
There was a problem hiding this comment.
@broccolinisoup would you want to include this as part of the breaking change or leave them separate?
As an alternative, we might be able to use the react-is package to check if it is a valid element type (like ReactIs.isValidElementType(Icon)) as a more stable way of passing props versus not
There was a problem hiding this comment.
Thanks for your great suggestion @joshblack! I removed the deprecated Octicons 🎉 Build and tests seems fine but let me know if there is anything I am missing to do here.
Also, I pushed it as a commit here, would that be better if I pushed another PR for it you think? 🤔
There was a problem hiding this comment.
If it is good to go within this PR, I'll update the title and the changeset. If not, I'll push another PR
Co-authored-by: Josh Black <joshblack@github.com>
broccolinisoup
changed the title
createIconComponent utility func to return React.forwardRef React.forwardRef & Remove support for Octicon
Closes #910
As stated in the issue itself, our icon components do not forward their refs and this is currently causing issues in the remediated Tooltip implementation primer/react#3032. It is also a great enhancement for future cases to make sure their refs are available to access when needed.
Re introducing this change as a major, please see the comment