carbon-addons-iot-react icon indicating copy to clipboard operation
carbon-addons-iot-react copied to clipboard

Published 20 hours ago • verified publisher icon carbon-design-system

[DashboardEditor] react-grid-layout 1.2.3 breaks click behavior

Open jkeen opened this issue 3 years ago • 5 comments

What package is this for?

  • [x ] React

Describe the bug

Upgrading past react-grid-layout version 1.2.2 causes the DashboardEditor tests to fail as soon as a card is clicked. The error that is thrown is a <DraggableCore> not mounted on DragStart! and is described/demonstrated further in this issue, and is due to the following change in react-grid-layout:

React-Grid-Layout is now fully compatible with <React.StrictMode>.
- Usage of ReactDOM has been removed by using React.createRef() inside `react-grid-layout`, and the new nodeRef prop in react-draggable.

To Reproduce

Steps to reproduce the behavior:

  1. Try updating to react-grid-layout 1.2.3 or greater
  2. Run the DashboardEditor tests
  3. See that failure

jkeen avatar Apr 06 '21 17:04 jkeen

I apologize for the breakage here, I did not understand fully how changing ref behavior would break userland.

To fix this, you need to ensure that Card forwards refs to the underlying DOM node.

Let me know if this does not work for you, we could consider adding an option to RGL so this breakage is avoided. This will, however, not work in React 17 once ReactDOM.findDOMNode() is removed.

STRML avatar May 10 '21 17:05 STRML

@jkeen Can you verify that this is still valid?

JordanWSmith15 avatar Jul 22 '21 17:07 JordanWSmith15

I tested and the issue is still present. Seems we have a suggested fix that we can implement, thanks @STRML. The fix is small but I think we need to test it carefully so that we don't introduce any regression bugs because of this.

We should probably fix this soon but until then the "workaround" is to not update the react-grid-layout lib.

bjornalm avatar Aug 09 '21 08:08 bjornalm

We've got a few layers of complication in this one. It'd be relatively simple to add forwardRefs to the cards, but unfortunately, we have our cards wrapped in react-visibility-sensor and react-sizeme--neither of which have been updated to use refs properly and are still relying on findDomNode, so this is blocked until those projects are updated. There is an open PR to update visibility sensor (https://github.com/joshwnj/react-visibility-sensor/pull/161), but nothing for react-sizeme.

kevinsperrine avatar Aug 30 '21 16:08 kevinsperrine

We may be able to revisit this one since we have an open PR to remove our dependency on size-me and visibility sensor.

davidicus avatar Dec 15 '21 20:12 davidicus