Skip to content

Conversation

@senadir
Copy link

@senadir senadir commented Sep 15, 2019

this is an initial PR to migrate from the now deprecated findDomNode to using ref

functionality wise this PR seems to work, there are however some problems that I could use some help in:

  • the flow types are not correct, I've never used flow before so I'm not sure how to fix and didn't have much time to research for it.
  • I couldn't run the tests from some reason (they run immediately but nothing happens).
  • I would rather refactor this to use forwardRef and use the same ref from Draggable into DraggableCore but it didn't work for some reason, but alas, the current approach still seems to work

@STRML
Copy link
Collaborator

STRML commented Oct 29, 2019

This is actually quite difficult to do well, because wrapped elements must properly forward refs down to the underlying DOM node, which is a very breaking change and will require significant modification on many apps that do something like:

<Draggable>
  <Panel>
    <Panel.Title>...</Panel.Title>
    <Panel.Body>...</Panel.Body>
  </Panel>
</Draggable>

I expect components to exert this kind of hygiene into React 17, but there will be some hard-to-solve edge cases.

@vkrol
Copy link

vkrol commented May 7, 2020

@STRML
Copy link
Collaborator

STRML commented May 8, 2020 via email

@vkrol
Copy link

vkrol commented May 9, 2020

@STRML If I understand correctly, this PR contains a similar solution.

@STRML
Copy link
Collaborator

STRML commented May 11, 2020

Unfortunately, it doesn't. The draggableRef and elementRef (why different names?) are ignored if passed in props.

@thienandangthanh
Copy link

#478 supersedes this PR.
I think this PR should be closed.

@senadir senadir closed this May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants