feat(Ref): extract to a separate package, add utils#1281
feat(Ref): extract to a separate package, add utils#1281layershifter merged 13 commits intomasterfrom
Conversation
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
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.
FYI @miroslavstastny publishConfig is defined
| 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.
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.
Only this line was changed: we don't need @ts-ignore more because MutableRefObject was introduced to typings
layershifter
added
⚙️ enhancement
| static propTypes = { | ||
| children: PropTypes.element.isRequired, | ||
| innerRef: customPropTypes.ref, | ||
| innerRef: customPropTypes.ref.isRequired as PropTypes.Validator<React.Ref<any>>, |
| @@ -1,7 +1,7 @@ | |||
| import { RefForward } from '@stardust-ui/react-component-ref' | |||
There was a problem hiding this comment.
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.
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.
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.
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.
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
|
|
||
| ### 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.
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...
mnajdova
left a comment
mnajdova
left a comment
There was a problem hiding this comment.
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
3 participants
Fixes #998.
This PR:
Refcomponent to@stardust-ui/react-component-refpackage, howeverRefcan be still accessed from@stardust-ui/reactisRefObject(),toRefObject()utils