Skip to content

Conversation

@austingreendev
Copy link
Contributor

Description

With the new React.createRef usages, some of our old "forward-ref" props have invalid prop-types. An example error:

// When supplying React.createRef() to `buttonRef`
Warning: Failed prop type: Invalid prop `buttonRef` of type `object` 
supplied to `IconButton`, expected `function`.

Detail

This PR corrects the invalid prop-types for the react-buttons package. There are also other usages of PropTypes.func for refs in additional packages, but they are currently deprecated. I am excluding them from this PR.

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 💅 view component styling is based on a Garden CSS
    component
  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • ♿ analyzed via axe and evaluated using VoiceOver
  • 💂‍♂️ includes new unit tests
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

@coveralls
Copy link

coveralls commented Jul 29, 2019

Coverage Status

Coverage remained the same at 95.463% when pulling bb95946 on agreen/button-ref-props into c257b6c on master.

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

I think these buttonRef propTypes just need to be removed as they are no longer valid for Button.

active: PropTypes.bool,
/** Callback for reference of the native button element */
buttonRef: PropTypes.func,
buttonRef: PropTypes.any,

Choose a reason for hiding this comment

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

Is any really the appropriate choice here? It should only be an object or a function, right?

In the other hand, if you actually want to enforce the type of an actual ref, for example a prop that is forwarded to another component's ref, you have to allow both on object shaped by React.createRef() (The preferred way to create ref in React components, see type in react code here) and function types.

Object shape: PropTypes.shape({ current: PropTypes.instanceOf(Element) }) is for refs created using React.createRef()
PropTypes.func is for callback refs as used in the above question
Which gives the following PropType definition:

forwardRef: PropTypes.oneOfType([
  PropTypes.func, 
  PropTypes.shape({ current: PropTypes.instanceOf(Element) })
])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have avoided the shape prop-type in other packages which is why I went with any here.

We ended up removing them as they actually don't do anything in the v6+ world.

@austingreendev austingreendev merged commit a18552a into master Jul 31, 2019
@austingreendev austingreendev deleted the agreen/button-ref-props branch July 31, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants