Skip to content
New issue

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

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

Already on GitHub? # to your account

Add forwarding of refs for Header component #3891

Closed
wants to merge 1 commit into from
Closed

Add forwarding of refs for Header component #3891

wants to merge 1 commit into from

Conversation

andyrichardson
Copy link

Partially addresses #3819

@welcome
Copy link

welcome bot commented Jan 31, 2020

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
  • Run yarn test locally to catch errors. This ensures all components still behave as they should.
  • Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
  • Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@layershifter
Copy link
Member

@andyrichardson hey!

This thing was missed, but it will be great to bump it. Are you still interested in it?

@andyrichardson
Copy link
Author

@layershifter for sure! I just need to get it passing in CI?

@layershifter
Copy link
Member

layershifter commented Aug 5, 2020

@andyrichardson great 👍 Yes, let me know if I can help somehow there.

P.S. please restore your fork as currently it's impossible to push changes 😺
image

@@ -159,4 +167,4 @@ Header.propTypes = {
Header.Content = HeaderContent
Header.Subheader = HeaderSubheader

export default Header
export default React.forwardRef(Header)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this change should happen in the definition i.e. const Header = React.forwardRef((props, ref) => {

  • we have statics (Header.Content, Header.Subheader) and it will not be usable on default export
  • a param for React.forwardRef() is a function, not component itself:

https://github.com/facebook/react/blob/41694201988c5e651f0c3bc69921d5c9717be88b/packages/react/src/ReactForwardRef.js#L38-L41

@layershifter
Copy link
Member

You will probably will have many CI issues (at least with componentInfo), let's merge master and start fixing them 🧑‍🏭

@layershifter
Copy link
Member

Closing for housekeeping 🏠 If you would like to follow up on it, feel free to create a new PR 🐱

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants