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

Update to react-virtual 2.10.4 #7213

Merged
merged 1 commit into from
Mar 10, 2025
Merged

Conversation

pjonsson
Copy link
Contributor

@pjonsson pjonsson commented Jun 23, 2024

What this PR does

Commit 236b9c9 locked the version
to 2.3.2 to avoid a bug in 2.4. The
bug in question has hopefully been
fixed in the past 3 years.

Test me

No idea how to test this.

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@zoran995
Copy link
Collaborator

zoran995 commented Oct 1, 2024

My previous tests regarding this was that this line is causing issue with react-virtual

const parentRef = React.createRef<HTMLDivElement>();

createRef is not valid in functional components and useRef should be used instead.

react-virtual is added to support search inside items, here is the PR #5112, maybe you can utilize the info from that PR to test this one. The other relevant doc are here https://docs.terria.io/guide/connecting-to-data/item-search/ and https://docs.terria.io/guide/connecting-to-data/item-search/indexed-item-search/. I am not aware of other easier way to really test this

@pjonsson
Copy link
Contributor Author

pjonsson commented Oct 1, 2024

const parentRef = React.createRef<HTMLDivElement>();

createRef is not valid in functional components and useRef should be used instead.

@zoran995 Thank you for the help. If you really mean that createRef is not valid in functional components, and SearchResults is a functional component, that sounds like an outright bug that should be fixed separately. Is there something subtle about it, or can createRef just be replaced with useRef in general?

@zoran995
Copy link
Collaborator

zoran995 commented Oct 3, 2024

They are the same just intended to be used in different type of components, here is react docs
https://react.dev/reference/react/createRef#migrating-from-a-class-with-createref-to-a-function-with-useref. It might be necessary just to pass null as default value to satisfy typescript.
But as everything in programming it would be good to test if everything works correctly

@zoran995
Copy link
Collaborator

@pjonsson there is an example link in #7290 that can be used for testing the item search tool.

@pjonsson pjonsson force-pushed the update-react-virtual branch from 59358a1 to 9be50d3 Compare December 19, 2024 16:30
@pjonsson
Copy link
Contributor Author

Rebased to latest main to resolve conflicts, and updated CHANGES.md.

@zoran995 zoran995 requested a review from ljowen December 19, 2024 16:45
@pjonsson pjonsson force-pushed the update-react-virtual branch from 9be50d3 to 85932a1 Compare February 20, 2025 20:58
@zoran995
Copy link
Collaborator

zoran995 commented Mar 7, 2025

Hi @pjonsson, can you rebase this on the latest main?

Commit 236b9c9 locked the version
to 2.3.2 to avoid a bug in 2.4. The
bug in question has hopefully been
fixed in the past 3 years.
@pjonsson pjonsson force-pushed the update-react-virtual branch from 85932a1 to 912e50e Compare March 8, 2025 10:46
@pjonsson
Copy link
Contributor Author

pjonsson commented Mar 8, 2025

@zoran995 sorry, big pile of stuff in the PR queue so hard to keep track of what needs to be rebased after things are merged. Just rebased this and #7411 that you approved yesterday.

Copy link
Contributor

@ljowen ljowen left a comment

Choose a reason for hiding this comment

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

Tested with @ykiu 's link locally

@zoran995 zoran995 merged commit 475436d into TerriaJS:main Mar 10, 2025
6 checks passed
@zoran995
Copy link
Collaborator

Thanks @pjonsson. Sometimes it’s easier for us to ping you about rebasing something than for you to constantly rebase everything.

@pjonsson pjonsson deleted the update-react-virtual branch March 10, 2025 09:09
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.

3 participants