Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v5] [table] fix: stop using findDOMNode in Draggable interactions #6137

Merged
merged 6 commits into from
May 9, 2023

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented May 8, 2023

Fixes #3979

Changes proposed in this pull request:

Migrate away from ReactDOM.findDOMNode() in the <Draggable> component by borrowing an implementation from <ResizeSensor> which clones its single React child and injects its own DOM ref. It's even simpler here than in ResizeSensor because we don't have to deal with a prop ref sent to <Draggable>.

@blueprintjs/core changelog

  • fix(ResizeSensor): account for props.targetRef if specified (previously, this component would not actually take DOM measurements properly if this prop was specified)
    • ⚠️ break: the type of the targetRef prop is now more specific, it only allows ref objects and no ref callbacks. Before this change, this prop's usage broke the component completely, so it's extremely unlikely that this break will impact any users negatively.
  • feat(EditableText): new prop elementRef allows users to pass through a DOM ref (useful for taking measurements or implementing interactions like <Draggable> in @blueprintjs/table)

@blueprintjs/table changelog

  • fix: stop using ReactDOM.findDOMNode() to implement drag & drop interactions, which unblocks support for React strict mode

Reviewers should focus on:

No regressions in dragging behavior in Table component demos

Screenshot

Note that there are some delays in re-rendering cells (until you scroll around a bit in some cases), but these are bugs present on develop too:

2023-05-09 15 08 36

@adidahiya
Copy link
Contributor Author

fix compile

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

plumb through targetRef

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya adidahiya changed the title [v5] [table] fix(Draggable): don't use findDOMNode [v5] [table] fix: stop using findDOMNode in Draggable interactions May 9, 2023
@@ -330,8 +319,6 @@ export class Popover<
targetTagName = "div";
}

const ref = mergeRefs(popperChildRef, this.targetRef);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer need to merge refs, since react-popper is already doing that under the hood: https://github.com/floating-ui/react-popper/blob/beac280d61082852c4efc302be902911ce2d424c/src/Reference.js#L17-L23

@@ -55,7 +55,7 @@ export interface ResizeSensorProps {
* If you attach a `ref` to the child yourself when rendering it, you must pass the
* same value here (otherwise, ResizeSensor won't be able to attach its own).
*/
targetRef?: React.Ref<any>;
targetRef?: React.RefObject<HTMLElement>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slight breaking change - see PR description for justification

@adidahiya
Copy link
Contributor Author

Fix Popover <> ResizeSensor

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

Fix test typecheck

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

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.

1 participant