fix(sdk): Remove ExplicitSize findDOMNode console errors #52253
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 6f02407...eb2e3f5.
|
bc8df3b to
ddce178
Compare
c16cdc2 to
74f81b7
Compare
3b6a883 to
51072de
Compare
b884898 to
474fc29
Compare
c608634 to
4377ea8
Compare
4377ea8 to
ea9e992
Compare
39493e8 to
9463545
Compare
| @@ -451,6 +457,7 @@ export function FieldValuesWidgetInner({ | |||
| minWidth: minWidth ?? undefined, | |||
| maxWidth: maxWidth ?? undefined, | |||
| }} | |||
| ref={ref} | |||
There was a problem hiding this comment.
I guess this would behave differently when the error message is shown, since getDomNode should return the error node, but in the real world, I don't know how much difference that would make. Can you just ensure the UI behavior are the same even when this component throw errors?
There was a problem hiding this comment.
I don't think it made much of a difference but just for completeness I've added a forwardRef to ErrorBoundary and made the corresponding changes. I think it's better that we have these explicitly passed (I mean... we have to if we want to get away from this error)
| "data-testid": testId, | ||
| children, | ||
| }: LoadingAndErrorWrapperProps) => { | ||
| export const LoadingAndErrorWrapper = forwardRef< |
There was a problem hiding this comment.
some of the files here aren't listed as the component that uses ExplicitSize, I assume these component are the top level component used in a component that uses ExplicitSize? That way we need to pass ref to the underlying component and we need to forward refs.
There was a problem hiding this comment.
yep, that's exactly it. I should have mentioned that - when I didn't use forwardRef on here, we would get even more console errors which would defeat the purpose of this PR
| children?: ReactNode; | ||
| } | ||
|
|
||
| const TabList = forwardRef(function TabGroup<T>( | ||
| { value, onChange, onScroll, children, ...props }: TabListProps<T>, | ||
| { value, onChange, onScroll, children, rootRef, ...props }: TabListProps<T>, |
There was a problem hiding this comment.
can't we wrap ref again like in other components, so we don't need an extra ref prop? I'm worried this will be hard to understand over time.
There was a problem hiding this comment.
let me see if I can do something about this. I hated this solution
There was a problem hiding this comment.
Ok, so apparently we can merge the tabListRef with the current ref and we should be ok. It looks like TabListRoot and TabListContent keep the same width
| <TabList | ||
| onChange={onChange as (value: unknown) => void} | ||
| onScroll={event => setScrollPosition(event.currentTarget.scrollLeft)} | ||
| rootRef={ref} |
There was a problem hiding this comment.
I think I already commented about this. What about we wrap ref instead of passing an additional ref prop? I find this inconsistent with other wrapped ref and the API isn't super clear.
frontend/src/metabase/query_builder/components/NativeQueryEditor/NativeQueryEditor.tsx
Show resolved
Hide resolved
...query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx
Outdated
Show resolved
Hide resolved
| @@ -1137,7 +1137,9 @@ class TableInteractive extends Component { | |||
| } = this.props; | |||
|
|
|||
| if (!width || !height) { | |||
| return <div className={className} />; | |||
| return ( | |||
| <div ref={el => this.props.forwardedRef(el)} className={className} /> | |||
There was a problem hiding this comment.
why can't we just simply pass ref?
| <div ref={el => this.props.forwardedRef(el)} className={className} /> | |
| <div ref={this.props.forwardedRef} className={className} /> |
| return ( | ||
| <TableInteractiveRoot | ||
| ref={el => this.props.forwardedRef?.(el)} |
| const VisualizationMemoized = memoizeClass("_getQuestionForCardCached")( | ||
| Visualization, | ||
| ); |
There was a problem hiding this comment.
why do we need to extract a component here? Can't we keep the HoC in the same _.compose?
There was a problem hiding this comment.
I was having trouble getting this to work because I kept running into a few errors that had to do with the memoizeClass function and for the life of me could not get it to work. I'm going to try something else but I'd prefer it to work
There was a problem hiding this comment.
I think it'd have something to do with the order you define HoCs. It should be possible to put this in connect and behave the same.
|
@metabase-bot backport release-x.52.x |
|
@oisincoveney Manual conflict resolution is required |
|
🚀 This should also be released by v0.53 |
Removes
console.erroraboutfindDOMNodebeing deprecated by removingfindDOMNodefromExplicitSize.findDOMNodeis a way for react to get the root HTML element of the component that's passed into that function, sofindDOMNode(this)gets the root element of the component in which it's called. To refactor this, we have to create a ref and explicitly pass that into the components that use this HOC.Nothing should change.
findDOMNodeerror messages forExplicitSizeshould be gone now. Manual testing is the way to go for now I thinkExplicitSizeis used in the following files, we need to explicitly pass arefin these components to replace this functionality.Some other files have been updated with
forwardRefsince we passrefs explicitly now to satisfy the fact that we need explicit refs to replacefindDOMNode