feat(Ref): implement `innerRef` on all components #1879

layershifter · 2017-07-20T10:42:21Z

WIP

Why?

ref isn't a prop, it has a same behaviour as key - it never passed to a component. However, there were numerous issues about refs (#1740, #1602, #1849, #1732 and more) and it's time to make something in this way. document.querySelector is a workaround, but it's not a solution.

Problem is a well known in React's world, there is a long-live issue facebook/react#4213, there was also the RFC facebook/react#6974 from Dan.

Variants

Project	Prop	Example
formsy-react-components	`componentRef` & `innerRef`	`refs.md`
office-ui-fabric-react	`componentRef`	office-ui-fabric-react#1356
react-redux	`connect` HOC, `withRef`	reduxjs/react-redux@`2d3d0be`
react-router	`innerRef`	`Link.md`

TODO

fix broken tests
update ref and trigger usages
update context and scrollContext usages

levithomason · 2017-07-21T20:43:56Z

Very cool, thanks for getting this going. It has been a long time running and will solve many issues. ❤️

I assume this PR will also apply this to all the problem areas noted in the original issue?

layershifter · 2017-07-21T21:03:01Z

Yep, however there is an unresolvable problem with a HOC.

const RefsSupported = Ref(RefsNotSupported)
<RefsSupported ref={node => console.log(node)} />

There is no way to make this working because ref is not a prop, so component doesn't know anything about it. I've read many issues about this in React's repository and there is no any progress. I think that we should add a componentRef or innerRef prop to each component like React Bootstrap team done. But I need to do more progress before I can make final proposals.

levithomason · 2017-07-22T01:44:55Z

Agreed, in my example on react.run I think I used withDOMNode={this.handleDOMNode} or something similar. My thought was that it will not return the ref, but the DOM node. Their site no longer works, so the example is not accessible.

codecov-io · 2017-07-24T17:20:42Z

Codecov Report

Merging #1879 into master will decrease coverage by 16.91%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master    #1879       +/-   ##
===========================================
- Coverage   99.73%   82.82%   -16.92%     
===========================================
  Files         152      147        -5     
  Lines        2656     2492      -164     
===========================================
- Hits         2649     2064      -585     
- Misses          7      428      +421

Impacted Files	Coverage Δ
src/modules/Dropdown/Dropdown.js	`75.77% <ø> (-24.23%)`	⬇️
src/elements/Button/Button.js	`100% <ø> (ø)`	⬆️
src/addons/TextArea/TextArea.js	`100% <ø> (ø)`	⬆️
src/modules/Modal/Modal.js	`83.78% <ø> (-16.22%)`	⬇️
src/hoc/withRef.js	`100% <100%> (ø)`
src/elements/Image/Image.js	`100% <100%> (ø)`	⬆️
src/addons/Ref/Ref.js	`100% <100%> (ø)`	⬆️
src/behaviors/Visibility/Visibility.js	`96.22% <100%> (-3.78%)`	⬇️
src/modules/Popup/Popup.js	`34.9% <100%> (-65.1%)`	⬇️
src/modules/Search/Search.js	`16.06% <0%> (-83.41%)`	⬇️
... and 151 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 6ef5af5...9fcdf3c. Read the comment docs.

layershifter · 2017-08-01T16:42:38Z

test/utils/shallow.js

+  }
+}
+
+export default (...args) => dive(new Wrapper(...args))


@levithomason I need you there.

After I injected withRef HOC to getElementType I received many broken tests. Half of then fixed with the code above, its idea to dive automatically when shallow is called.

Another half shoud be fixed with overriding of the shallow method on ShallowWrapper, but the inheritance doesn't work. WTF? Can you explain what I'm doing wrong? I think that I'm missing something basic, but at now it destroys my mind. phantomjs doesn't support neither Proxy, nor __noSuchMethod__ 😢

Example test:

shallow(<Confirm cancelButton='foo' />) .find('Button') .first() .shallow() .should.have.text('foo')

expected <Button /> to have text 'foo', but it has '<refHOC />'

const shallowMethod = ShallowWrapper.prototype.shallow ShallowWrapper.prototype.shallow = function (...args) { return dive(shallowMethod.apply(this, ...args)) }

I found way to solve this, looks ugly, but works. Open for better solution

layershifter

I returned to this PR and want to finish it before this weekend because I want to get initial review on it. I fixed all elements tests, will back to all other tomorrow.

layershifter · 2017-09-01T13:11:29Z

@levithomason I didn't finished this PR, but I want to get your review. There are many things that don't make me happy.

getElementType

Now it returns an withRef hoc instead of a tag. It's not fatal, but requires the rewrite of code where we relied on tagName like there and there.

withRef

This hoc contains magic conditions. I'm not sure that it's the best idea, but calls of findDOMNode aren't performance-free and we should omit them.

Tests

Most of test are broken because now we get withRef hoc instead of component when we do shallow or mount. I've made shallow wrapper, but it also adds problems:

we can't use state() on it
strange differences after it
broken find()

@levithomason I can fix all left tests, but I need comments and signal from you, that I've chosen the right way 😄

levithomason · 2017-09-01T16:10:55Z

src/hoc/withRef.js

+
+import Ref from '../addons/Ref'
+
+const withRef = Child => class refHOC extends Component {


Capitalized classes, please.

nit: Could also just be WithRef

Nice catch 👍

levithomason · 2017-09-01T16:14:38Z

src/lib/getElementType.js

@@ -19,7 +21,7 @@ function getElementType(Component, props, getDefault) {
  // computed default element type

  if (getDefault) {
-    const computedDefault = getDefault()
+    const computedDefault = typeof getDefault === 'function' ? getDefault() : getDefault


If this change is made, the signature needs to be updated as well.

* @param {function} [getDefault] A function that returns a default element type.

That said, I'm not sure there is too much advantage to allowing a conditional here.

Yes, I used original as you wrote below, so this change was reverted.

levithomason · 2017-09-01T16:20:02Z

Now it returns an withRef hoc instead of a tag. It's not fatal, but requires the rewrite of code where we relied on tagName like there and there.

As for the image example, this is actually a bug :) The 3rd argument is a function:

-const ElementType = getElementType(Image, props, wrappedImage && 'div')
+const ElementType = getElementType(Image, props, () => wrappedImage && 'div')

getElementType.js

* @param {function} [getDefault] A function that returns a default element type.

As for the list item example, I'm not entirely sure right now. One idea, but feels hacky :( is to have withRef add the Child component to the refHOC as a static, like originalComponent. Then, we could check it something like:

<Button as='div' />

const Button (props) => {
  const ElementType = getElementType(Button, props)

  // given the above invocation:
  // ElementType.originalComponent === 'div'

  return <ElementType />
}

I'm not super fond of this, but it seems like one of the only possibilities for "unwrapping" the HOC and accessing the original component.

levithomason · 2017-09-01T16:20:28Z

I will have to come back and address the others another time!

levithomason · 2017-09-02T14:21:20Z

Note, context props should also use the Ref component. This way, other React elements can be passed as context nodes. See #1978 (comment).

layershifter · 2017-09-29T07:30:19Z

Requires #2142.

levithomason · 2017-09-30T20:21:22Z

Unblocked, has some conflicts.

…-Org/Semantic-UI-React into feat/ref # Conflicts: # src/addons/Ref/Ref.d.ts # src/addons/Ref/Ref.js # src/collections/Breadcrumb/BreadcrumbSection.js # src/index.js # test/specs/addons/Ref/Ref-test.js # test/specs/addons/Ref/fixtures.js

layershifter · 2017-10-06T10:19:39Z

Needs #2144.

levithomason · 2017-10-10T05:20:54Z

#2144 is now unblocked.

levithomason · 2017-10-29T16:00:29Z

Note, #2144 was merged and this PR should be clear to move forward.

…React into feat/ref

…-Org/Semantic-UI-React into feat/ref

layershifter added the enhancement label Jul 20, 2017

layershifter force-pushed the feat/ref branch from 2743e97 to 5307f11 Compare July 24, 2017 17:20

layershifter force-pushed the feat/ref branch 2 times, most recently from 574158a to f1e6df7 Compare August 1, 2017 16:33

layershifter commented Aug 1, 2017

View reviewed changes

layershifter mentioned this pull request Aug 11, 2017

docs(Button): add example with replicating button behaviour #1951

Merged

This was referenced Aug 13, 2017

fix(popup): calculate trigger position on portal mount for manual ope… #1190

Closed

Feature request: context prop to Visibility component #1959

Closed

layershifter mentioned this pull request Aug 18, 2017

Can I safely use Input.prototype.inputRef? #1980

Closed

layershifter mentioned this pull request Aug 30, 2017

chore(tests): replace phantomjs with chromium #2024

Merged

layershifter and others added 2 commits August 31, 2017 18:53

feat(Ref): add component

5dfc617

feat(Ref): add component

48c6b57

layershifter force-pushed the feat/ref branch from 9955bb1 to c990b34 Compare August 31, 2017 15:54

layershifter commented Aug 31, 2017

View reviewed changes

layershifter added the ready for review label Sep 1, 2017

levithomason reviewed Sep 1, 2017

View reviewed changes

levithomason mentioned this pull request Sep 2, 2017

feat(Sticky|Visibility): add scroll context #1978

Merged

layershifter mentioned this pull request Sep 7, 2017

Sticky in Scrolling Modal doesn't work properly in Chrome ! #2054

Closed

Alexander Fedyashov added 3 commits September 11, 2017 18:15

style(mixed): fix lint issues

c7ebfe4

fix(mixed): fix broken tests

1f7ef3e

fix(shallow): fix shallow test util

a1bad06

layershifter removed the needs author feedback label Sep 29, 2017

layershifter mentioned this pull request Sep 29, 2017

test(mixed): add test for content prop, remove useless tests #2144

Merged

levithomason removed the blocked label Sep 30, 2017

layershifter force-pushed the feat/ref branch 2 times, most recently from 2317aa1 to 11fef64 Compare October 2, 2017 07:17

Alexander Fedyashov added 2 commits October 2, 2017 11:33

test(mixed): remove useless content tests

d09f519

wip

df2f197

layershifter force-pushed the feat/ref branch from 11fef64 to df2f197 Compare October 2, 2017 12:55

layershifter changed the title ~~feat(Ref): implement innerRef on all our components~~ feat(Ref): implement innerRef on all components Oct 2, 2017

layershifter added the blocked label Oct 6, 2017

layershifter mentioned this pull request Oct 11, 2017

Triggering Bubbled Component Events #2164

Closed

2 tasks

layershifter removed the blocked label Oct 17, 2017

merge with master

3bfbadb

layershifter mentioned this pull request Oct 19, 2017

feat(Portal): use Ref for handling trigger refs #2219

Merged

layershifter added 4 commits November 5, 2017 11:46

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

807cdcd

…React into feat/ref

Merge branches 'feat/ref' and 'master' of https://github.com/Semantic…

7f7b8ad

…-Org/Semantic-UI-React into feat/ref

fix(Image): make Image stateless again

c1a840d

test(common): restore tests

20bbbfa

layershifter force-pushed the feat/ref branch from fc6cee3 to 20bbbfa Compare November 5, 2017 12:03

feat(innerRef): use ElementType as child

9fcdf3c

layershifter mentioned this pull request Nov 7, 2017

Dealing with Button as a NavLink of react router gives a react warning #2295

Closed

layershifter closed this Nov 9, 2017

layershifter mentioned this pull request Nov 9, 2017

feat(ElementType): new wrapper for all elements #2306

Closed

layershifter deleted the feat/ref branch May 8, 2018 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Ref): implement `innerRef` on all components #1879

feat(Ref): implement `innerRef` on all components #1879

layershifter commented Jul 20, 2017 •

edited

Loading

levithomason commented Jul 21, 2017

layershifter commented Jul 21, 2017

levithomason commented Jul 22, 2017

codecov-io commented Jul 24, 2017 •

edited

Loading

layershifter Aug 1, 2017

layershifter Aug 31, 2017

layershifter left a comment •

edited

Loading

layershifter commented Sep 1, 2017

levithomason Sep 1, 2017 •

edited

Loading

layershifter Sep 11, 2017

levithomason Sep 1, 2017 •

edited

Loading

layershifter Sep 11, 2017

levithomason commented Sep 1, 2017 •

edited

Loading

levithomason commented Sep 1, 2017

levithomason commented Sep 2, 2017

layershifter commented Sep 29, 2017

levithomason commented Sep 30, 2017

layershifter commented Oct 6, 2017

levithomason commented Oct 10, 2017

levithomason commented Oct 29, 2017


		import Ref from '../addons/Ref'

		const withRef = Child => class refHOC extends Component {

feat(Ref): implement innerRef on all components #1879

feat(Ref): implement innerRef on all components #1879

Conversation

layershifter commented Jul 20, 2017 • edited Loading

WIP

Why?

Variants

TODO

levithomason commented Jul 21, 2017

layershifter commented Jul 21, 2017

levithomason commented Jul 22, 2017

codecov-io commented Jul 24, 2017 • edited Loading

Codecov Report

layershifter Aug 1, 2017

Choose a reason for hiding this comment

layershifter Aug 31, 2017

Choose a reason for hiding this comment

layershifter left a comment • edited Loading

Choose a reason for hiding this comment

layershifter commented Sep 1, 2017

getElementType

withRef

Tests

levithomason Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

layershifter Sep 11, 2017

Choose a reason for hiding this comment

levithomason Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

layershifter Sep 11, 2017

Choose a reason for hiding this comment

levithomason commented Sep 1, 2017 • edited Loading

levithomason commented Sep 1, 2017

levithomason commented Sep 2, 2017

layershifter commented Sep 29, 2017

levithomason commented Sep 30, 2017

layershifter commented Oct 6, 2017

levithomason commented Oct 10, 2017

levithomason commented Oct 29, 2017

feat(Ref): implement `innerRef` on all components #1879

feat(Ref): implement `innerRef` on all components #1879

layershifter commented Jul 20, 2017 •

edited

Loading

codecov-io commented Jul 24, 2017 •

edited

Loading

layershifter left a comment •

edited

Loading

levithomason Sep 1, 2017 •

edited

Loading

levithomason Sep 1, 2017 •

edited

Loading

levithomason commented Sep 1, 2017 •

edited

Loading