-
Notifications
You must be signed in to change notification settings - Fork 10
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
Yuhsuan/react 18 upgrade #2426
Yuhsuan/react 18 upgrade #2426
Conversation
…ct-split-pane type def
Updates:
Update: we could consider replacing |
nice work @YuHsuan-Hwang 😎 🚀 |
Note: Let's close #1974 after this PR is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good on Mac and Ubuntu.
Filed #2433 for the |
Update: fixed the issue of auto scrolling not being triggered (found by e2e tests) |
@@ -353,7 +353,7 @@ export class FilterableTableComponent extends React.Component<FilterableTableCom | |||
return ( | |||
<Table2 | |||
className={className} | |||
ref={table.updateTableRef ? ref => table.updateTableRef(ref) : null} | |||
ref={table.updateTableRef ?? null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From React documentation:
React will also call your ref callback whenever you pass a different ref callback. In the above example, (node) => { ... } is a different function on every render. When your component re-renders, the previous function will be called with null as the argument, and the next function will be called with the DOM node.
Therefore, FilterableTableComponent
updates the ref whenever the table re-renders because there is an additional arrow function in the ref callback.
Moreover, the ref is set to null and then to the DOM node in the process. Before react 18, we use the ref after the two calls are complete. With react 18, the re-render timing somehow changes and we use the ref between the two calls —— the ref is (temporarily) null and the auto scroll is not triggered.
I remove the additional arrow function to avoid unnecessary reset of the ref, avoiding the entire issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks fine after some manual testing of konva related things. The e2e tests have some failures due to tooltips blocking element to be clicked. The FOV of multi-color blending on load changes due to performance changes while running e2e tests. These issues are addressed in an e2e branch.
Description
Patches
golden-layout
and upgrades to react 18.Also
react-color
type def (somehow does not work with react 18 type def)react-split-pane
type def (ref)react-konva
for react 18 compatibilitychildren
to react component props type (ref)Note that there is a warning from
react-resize-detector
. Investigated if it can be fixed by upgrading the package to v11, but found that the child function pattern<ReactResizeDetector/>
is removed from v10. Only theuseResizeDetector
hook is supported, which can only applied with functional components.This can be solved by creating a wrapper component, but requires additional changes because it adds an extra div element which breaks the current structure.I suggest we work on the upgrade in a separate PR (perhaps the next dev cycle) and focus ongolden-layout
patching and react 18 upgrade in this branch.Update: we could consider replacing
react-resize-detector
with blueprint<ResizeSensor/>
+ throttling.Let's close #1974 after this PR is merged.
Checklist
For linked issues (if there are):assignee and label addedGitHub Project estimate addedFor the pull request:
changelog updated/ no changelog update neededunit test added (for functions with no dependenies)API documentation added (for public variables and methods in stores)For dependencies:
new e2e test createdprotobuf version bumped/ no protobuf version bumped neededprotobuf updated to the latest dev commit/ no protobuf update neededcorresponding ICD test fix added (/ no ICD test fix needed (BackendService
changed)BackendService
unchanged)user manual prepared (for large new features)