feat(Ref): extract to a separate package, add utils #1281

layershifter · 2019-05-02T12:40:23Z

Fixes #998.

This PR:

moves Ref component to @stardust-ui/react-component-ref package, however Ref can be still accessed from @stardust-ui/react
adds isRefObject(), toRefObject() utils

…github.com/stardust-ui/react into feat/extract-ref-compomnent

codecov · 2019-05-03T08:25:00Z

Codecov Report

Merging #1281 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
packages/react/src/components/Input/Input.tsx	`100% <ø> (ø)`	⬆️
...ackages/react/src/components/Dropdown/Dropdown.tsx	`87.75% <ø> (ø)`	⬆️
...ckages/react/src/components/Popup/PopupContent.tsx	`90.9% <ø> (ø)`	⬆️
packages/react-component-ref/src/RefForward.tsx	`100% <ø> (ø)`
...react/src/components/RadioGroup/RadioGroupItem.tsx	`100% <ø> (ø)`	⬆️
packages/react-component-ref/src/RefFindNode.tsx	`100% <ø> (ø)`
packages/react/src/components/Dialog/Dialog.tsx	`0% <ø> (ø)`	⬆️
...ages/react/test/specs/commonTests/isConformant.tsx	`93.39% <ø> (ø)`	⬆️
packages/react/src/components/Popup/Popup.tsx	`68.29% <ø> (ø)`	⬆️
packages/react/src/components/Portal/Portal.tsx	`95% <ø> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 614a411...477e532. Read the comment docs.

DustyTheBot · 2019-05-03T08:34:26Z

	Warnings
⚠️	New package.json added: packages/react-component-ref/package.json. Make sure you have approval before merging!
⚠️	Package (or peer) dependencies changed. Make sure you have approval before merging!

Changed dependencies in packages/react/package.json

package	before	after
@stardust-ui/react-component-ref	-	^0.29.1

Generated by 🚫 dangerJS

…github.com/stardust-ui/react into feat/extract-ref-compomnent

layershifter · 2019-05-03T09:29:21Z

packages/react-component-ref/package.json

+  },
+  "publishConfig": {
+    "access": "public"
+  },


FYI @miroslavstastny publishConfig is defined

layershifter · 2019-05-03T09:29:45Z

packages/react-component-ref/src/Ref.tsx

@@ -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'


nit: To be common with other imports

layershifter · 2019-05-03T09:30:38Z

packages/react-component-ref/src/handleRef.ts

+  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


Only this line was changed: we don't need @ts-ignore more because MutableRefObject was introduced to typings

mnajdova · 2019-05-06T15:46:57Z

packages/react-component-ref/src/RefFindNode.tsx

@@ -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>>,


mnajdova · 2019-05-06T15:50:40Z

packages/react-component-ref/test/RefForward-test.tsx

@@ -1,7 +1,7 @@
+import { RefForward } from '@stardust-ui/react-component-ref'


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?

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

mnajdova · 2019-05-06T15:51:08Z

packages/react-component-ref/test/handleRef-test.ts

@@ -1,5 +1,5 @@
+import { handleRef } from '@stardust-ui/react-component-ref'


See the prev comment for the import

mnajdova · 2019-05-06T15:53:16Z

packages/react-component-ref/src/toRefObject.ts

@@ -0,0 +1,27 @@
+import * as React from 'react'
+
+const nullRefObject: React.RefObject<null> = { current: null }


What if the user uses reference like this in the following way: ref.current.focus()? How is it safe to have a nullRefObject?

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; }

mnajdova · 2019-05-06T16:18:30Z

CHANGELOG.md

@@ -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))


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

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

feat(Ref): extract to a separate package, add utils

a4df1d5

layershifter requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners May 2, 2019 12:40

layershifter added 2 commits May 2, 2019 14:44

add mergeRefs test

bd21d5c

fix dependencies

45fe17a

layershifter added the 🚧 WIP label May 2, 2019

layershifter added 2 commits May 3, 2019 10:12

Merge branches 'feat/extract-ref-compomnent' and 'master' of https://…

1a4ddc2

…github.com/stardust-ui/react into feat/extract-ref-compomnent

fix package.json for react-component-ref

f20ae30

layershifter added 5 commits May 3, 2019 10:37

remove pkg/ from .gituignore

374cdbb

revert type change

a345e74

revert propTypes change

627e8fe

Merge branches 'feat/extract-ref-compomnent' and 'master' of https://…

00a7407

…github.com/stardust-ui/react into feat/extract-ref-compomnent

remove @ts-ignore

8ad9110

layershifter commented May 3, 2019

View reviewed changes

layershifter mentioned this pull request May 3, 2019

feat(Popup): add mountNode and mountDocument #1288

Merged

add changelog entry

dff2746

layershifter added ⚙️ enhancement New feature or request 🚀 ready for review and removed 🚧 WIP labels May 6, 2019

mnajdova reviewed May 6, 2019

View reviewed changes

mnajdova approved these changes May 7, 2019

View reviewed changes

layershifter added 2 commits May 7, 2019 10:31

remove mergeRefs

0f8cf8c

Merge branch 'master' of https://github.com/stardust-ui/react into fe…

477e532

…at/extract-ref-compomnent # Conflicts: # CHANGELOG.md

layershifter merged commit 7eac364 into master May 7, 2019

delete-merged-branch bot deleted the feat/extract-ref-compomnent branch May 7, 2019 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Ref): extract to a separate package, add utils #1281

feat(Ref): extract to a separate package, add utils #1281

layershifter commented May 2, 2019 •

edited

codecov bot commented May 3, 2019 •

edited

DustyTheBot commented May 3, 2019 •

edited

layershifter May 3, 2019

layershifter May 3, 2019

layershifter May 3, 2019

mnajdova May 6, 2019

mnajdova May 6, 2019

layershifter May 6, 2019

mnajdova May 6, 2019

mnajdova May 6, 2019

layershifter May 6, 2019

mnajdova May 6, 2019

mnajdova left a comment

		@@ -1,7 +1,7 @@
		import { RefForward } from '@stardust-ui/react-component-ref'

		@@ -1,5 +1,5 @@
		import { handleRef } from '@stardust-ui/react-component-ref'

		@@ -0,0 +1,27 @@
		import * as React from 'react'

		const nullRefObject: React.RefObject<null> = { current: null }

feat(Ref): extract to a separate package, add utils #1281

feat(Ref): extract to a separate package, add utils #1281

Conversation

layershifter commented May 2, 2019 • edited

codecov bot commented May 3, 2019 • edited

Codecov Report

DustyTheBot commented May 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

layershifter commented May 2, 2019 •

edited

codecov bot commented May 3, 2019 •

edited

DustyTheBot commented May 3, 2019 •

edited