Skip to content

fix(buttons): allow correct ref validation in PropTypes #396

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 31, 2019
Merged

fix(buttons): allow correct ref validation in PropTypes #396

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf2fc4a
fix(buttons): allow correct ref validation in PropTypes
Jul 29, 2019
bb95946
Remove invalid propTypes
Jul 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/buttons/src/views/icon-button/ChevronButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ ChevronButton.propTypes = {
hovered: PropTypes.bool,
active: PropTypes.bool,
/** Callback for reference of the native button element */
buttonRef: PropTypes.func,
buttonRef: PropTypes.any,
Copy link

@dclowd9901 dclowd9901 Jul 30, 2019

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

@austingreendev austingreendev Jul 30, 2019

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.

/** Rotates icon 180 degrees */
rotated: PropTypes.bool
};
Expand Down
2 changes: 1 addition & 1 deletion packages/buttons/src/views/icon-button/IconButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ IconButton.propTypes = {
hovered: PropTypes.bool,
active: PropTypes.bool,
/** Callback for reference of the native button element */
buttonRef: PropTypes.func
buttonRef: PropTypes.any
};

IconButton.defaultProps = {
Expand Down