-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Ref): extract to a separate package, add utils #1281
Conversation
…github.com/stardust-ui/react into feat/extract-ref-compomnent
Codecov Report
@@ Coverage Diff @@
## master #1281 +/- ##
==========================================
+ Coverage 72.73% 72.79% +0.05%
==========================================
Files 736 738 +2
Lines 5633 5645 +12
Branches 1630 1655 +25
==========================================
+ Hits 4097 4109 +12
Misses 1529 1529
Partials 7 7
Continue to review full report at Codecov.
|
Changed dependencies in
Generated by 🚫 dangerJS |
}, | ||
"publishConfig": { | ||
"access": "public" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @miroslavstastny publishConfig
is defined
@@ -1,14 +1,14 @@ | |||
import * as customPropTypes from '@stardust-ui/react-proptypes' | |||
import * as PropTypes from 'prop-types' | |||
import * as React from 'react' | |||
import { isForwardRef } from 'react-is' | |||
import * as ReactIs from 'react-is' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: To be common with other imports
if (ref !== null && typeof ref === 'object') { | ||
// The `current` property is defined as readonly, however it's a valid way because | ||
// `ref` is a mutable object | ||
;(ref as React.MutableRefObject<N>).current = node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only this line was changed: we don't need @ts-ignore
more because MutableRefObject
was introduced to typings
@@ -20,10 +21,10 @@ export default class RefFindNode extends React.Component<RefFindNodeProps> { | |||
|
|||
static propTypes = { | |||
children: PropTypes.element.isRequired, | |||
innerRef: customPropTypes.ref, | |||
innerRef: customPropTypes.ref.isRequired as PropTypes.Validator<React.Ref<any>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1,7 +1,7 @@ | |||
import { RefForward } from '@stardust-ui/react-component-ref' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that we have inconsistency between the imports in these tests vs the imports in the components' tests. Can we change this import or is there a reason why it is like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to test public APIs and get rid of src
alias: it creates issues with IDE and works only in Jest/Webpack.
/cc @kuzhelov
@@ -1,5 +1,5 @@ | |||
import { handleRef } from '@stardust-ui/react-component-ref' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the prev comment for the import
@@ -0,0 +1,27 @@ | |||
import * as React from 'react' | |||
|
|||
const nullRefObject: React.RefObject<null> = { current: null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user uses reference like this in the following way: ref.current.focus()? How is it safe to have a nullRefObject
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be null
by typings:
interface RefObject<T> {
readonly current: T | null;
}
// Mutable it used in hooks an initial value is obviously defined
// React.useRef(null)
interface MutableRefObject<T> {
current: T;
}
CHANGELOG.md
Outdated
@@ -17,6 +17,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
|
|||
## [Unreleased] | |||
|
|||
### Features | |||
- `Ref` component extracted to a `@stardust-ui/react-component-ref` @layershifter ([#1281](https://github.com/stardust-ui/react/pull/1281)) | |||
- added `isRefObject()`, `toRefObject()` and `unstable_mergeRefs()` utils for React refs @layershifter ([#1281](https://github.com/stardust-ui/react/pull/1281)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not sure about this method, why even adding it with the unstable_
prefix? Is this blocker on client's side? I really don't see the things mention in the PR description as a valid points for adding it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the changes. As agreed offline, we will not add the unstable_mergeRefs()
for now, until we are certain of it's API and usage.
…at/extract-ref-compomnent # Conflicts: # CHANGELOG.md
Fixes #998.
This PR:
Ref
component to@stardust-ui/react-component-ref
package, howeverRef
can be still accessed from@stardust-ui/react
isRefObject()
,toRefObject()
utils