Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

Create and initialise default ref prop for Startdust components #417

Closed
silviuaavram opened this issue Nov 2, 2018 · 7 comments
Closed
Labels
⚙️ enhancement New feature or request vsts Paired with ticket in vsts
Projects

Comments

@silviuaavram
Copy link
Collaborator

silviuaavram commented Nov 2, 2018

Feature Request

Create default ref for Startdust components that should point to the first DOM Element in the renderComponent.

Problem description

While developing the Dropdown component and using other Stardust components, I've used Input and List and had to manually expose refs for those components in order to use them in the Dropdown implementation (input focus, list passed to Downshift).

Proposed solution

A default ref prop should be added for each component probably in the renderComponent and initialised with the first DOMElement from the render function. (the List should've had the ElementType <ul>).

This ref prop should also be easy to override in case we want custom behaviour.

This default ref prop should not impact the addition of other ref-like props. For instance, in Input, we can still have the inputRef pointing to the <input> and the default ref pointing to the <div> enclosing it.

@layershifter
Copy link
Member

There is a long standing issue with similar thing in SUIR, Semantic-Org/Semantic-UI-React#2844. There is also working prototype that uses forwardRef() API. It allows to solve following issues:

  • when user passes an own component via as prop
  • issues with functional components that cannot have refs

I should mention that it should be fully compatible with new React.createRef() API, because the current implementation of Ref component doesn't support it and will failure with ref is not a function:

const ref = React.createRef()
typeof ref // is object

This is a relevant thing for simple components. But is it the expected behaviour for the Dropdown component? What node will be a root in Popup?

@pkumarie2011 pkumarie2011 added the vsts Paired with ticket in vsts label Nov 2, 2018
@layershifter
Copy link
Member

layershifter commented Nov 5, 2018

Also I should note that findDOMNode() is deprecated in StrictMode and can be suddenly removed.

https://reactjs.org/docs/react-dom.html#finddomnode


I played around it, https://codesandbox.io/s/6n2z4o7jr Looks as correct solution, but not sure.

@layershifter
Copy link
Member

layershifter commented Nov 7, 2018

What we actually know?

  • Functional components doesn't support refs, it will cause warning
  • Class components support refs, but doesn't forward them to the element
  • All components that use forwardRef API are our friends

The PR Semantic-Org/Semantic-UI-React#2844 had a little problem, internal components will be able to forward refs with a composition and no any need to wrap them somehow.

Here is example of the wrong tree:

<Button as={List.Item} ref={buttonRef} /> // =>

<Button ref={buttonRef}>
  <Ref innerRef={buttonRef}>
    <List.Item forwaredRef={buttonRef} />
  </Ref>
</Button>

However, there is a simple solution if we will wrap all our components with forwardRef() on the top level, this will mean that all our components will satisfy isForwardRef() calls: https://codesandbox.io/s/7z6zvlp021

This means that forwardFunctionFactory() from Semantic-Org/Semantic-UI-React#2844 can be rewritten, I've updated it: https://github.com/Semantic-Org/Semantic-UI-React/pull/2844/files#diff-1bdf95bbff68e1181fa43b31de3fb2fe

Needs more checks but now we have a thing for discussion.

@layershifter
Copy link
Member

I also should note that forwardRef() simply returns an object:

return {
  $$typeof: REACT_FORWARD_REF_TYPE,
  render,
};

https://github.com/facebook/react/blob/v16.5.2/packages/react/src/forwardRef.js#L42

@layershifter
Copy link
Member

layershifter commented Nov 13, 2018

Q1: Should we add automatically refs?

This means that a component will receive a ref prop:

<Button ref={node => console.log(node)} /> // will point to `button` element

I should note that React's tree will be changed:

<ForwardRef>
  <Button />
</ForwardRef>

Q2: Should we add automatic refs to all our components?

This means that some our components will be an edge case:

<Button ref={node => console.log(node)} /> // will point to `button` element
<Input ref={node => console.log(node)} /> // will point to `input` element

<Popup ref={node => console.log(node)} /> // will point to what???

As proposed solution:

<Popup
  ref={node => console.log(node)} // throw a warning and will give `null`
  contentRef={node => console.log(node)} // will point to `div`
  trigger={<Button />}
  triggerRef={node => console.log(node)} // will point to `button`
/>

@layershifter
Copy link
Member

layershifter commented Nov 19, 2018

This comment describes our solutions to this issue.


Automatic refs

All our components will implement automatic refs that will point to matching DOM elements.

<Button ref={node => console.log(node)} /> // will point to `button` element
<Input ref={node => console.log(node)} /> // will point to `input` element

Background

Yes, it violates React's behaviour when you will have an instance of a component in ref, but we considered that in 99% cases you need an DOM element and a component's instance is useless for you.

When you're dealing with an instance you're starting to expore DOM methods like focus.

This also solves your problem with libraries like react-virtualized that relies on ref for computations.

<Segment ref={(node) => {
  node.focus() // will fail now
  node.getBoundingClientRect() // and this too
}} />

Edge cases

We decided to go with this API for complex components and we will not expose refs for component parts. You can use shorthand API for this:

<Popup
  // Will throw a warning and will give `null`
  // Use refs to component's parts via shorthands
  ref={node => console.log(node)}
/>

<Popup
  // Use shorthand API there, it will point to `div`
  content={{ content: 'Foo', ref: node => console.log(node)}}
  // We just clone element, please you refs there
  trigger={<Button ref={node => console.log(node)} />}
  // Your component doesn't support refs? Use Ref!
  trigger={(
    <Ref innerRef={node => console.log(node)} />
      <MaterialButton />
    </Ref>
  )}
/>

Implementation

[x] handleRef()

This util will allow us to handle both functional and createRef() refs. Done in #459.

[x] Ref uses forwardRef() by default. Done in #491.

findDOMNode() is deprecated in StrictMode and we can't rely on it, it also forces us to use react-dom.

We should to redesign this component to use forwardRef API in the most cases and use findDOMNode() only as a fallback. So, if React's team will decide to remove findDOMNode() it will be not issue for us.

[ ] Implement forwardRefFactory() and noForwardRefFactory()

Cherry pick code from Semantic-Org/Semantic-UI-React#2844 and make it working for Stardust UI.

  • implement factories
  • cover with unit tests
  • wrap all components and update common tests
  • use factories in createComponent()

@layershifter
Copy link
Member

#587 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⚙️ enhancement New feature or request vsts Paired with ticket in vsts
Projects
No open projects
Core Team
  
layershifter
Development

No branches or pull requests

4 participants