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 iTwin 4.7.2 (and newer appui) #73

Merged
merged 9 commits into from
Jun 25, 2024
Merged

Conversation

tcobbs-bentley
Copy link
Member

For some unknown reason, this caused our three remaining class-based React components to fail to compile, so those were converted to functional components.

For some unknown reason, this caused our three remaining class-based React components to fail to compile, so those were converted to functional components.
@tcobbs-bentley tcobbs-bentley requested a review from a team as a code owner June 24, 2024 23:49
@tcobbs-bentley
Copy link
Member Author

@toddsouthenbentley Please carefully review the class to functional React component conversions. I think I tested them, but I'm somewhat uncomfortable about things like the fact that the useEffect in TouchCaptor triggers every time isTouchStarted changes. It needs to do that, but since that removes the old listeners and creates new ones, I'm a little leery of it.

@toddsouthenbentley
Copy link
Contributor

@toddsouthenbentley Please carefully review the class to functional React component conversions. I think I tested them, but I'm somewhat uncomfortable about things like the fact that the useEffect in TouchCaptor triggers every time isTouchStarted changes. It needs to do that, but since that removes the old listeners and creates new ones, I'm a little leery of it.

It looks like appui-react now has a usePointerCaptor hook that works for both mouse and touch events. I would suggest using that and getting rid of the Touch* classes/functions.

If you're still leery of the new code then I'd suggest staying with the old code and figuring out why it no longer builds.

@tcobbs-bentley
Copy link
Member Author

It looks like appui-react now has a usePointerCaptor hook that works for both mouse and touch events. I would suggest using that and getting rid of the Touch* classes/functions.

It's not at all obvious how I would update the code to use that, and it looks like that would be an even bigger rewrite than converting a class to a function.

If you're still leery of the new code then I'd suggest staying with the old code and figuring out why it no longer builds.

I am investigating that, but the reality is that changing the class components to function components is a good thing. Given that everything works (resizing the resizable bottom panels), I think my conversion is correct.

@tcobbs-bentley
Copy link
Member Author

Updating @types/react to 17 seems to fix the problem that prevented class-based components from being used. I can revert the class-to-functional changes, but since they are generally a good thing, I'm a bit hesitant to do so.

@tcobbs-bentley
Copy link
Member Author

@toddsouthenbentley As you noticed, I reverted the components I was leery about to their original class versions after fixing the problem causing the compilation errors. I left ActionSheetButton as a functional component because it was much simpler and I don't have reservations about it.

@tcobbs-bentley tcobbs-bentley enabled auto-merge (squash) June 25, 2024 22:51
@tcobbs-bentley tcobbs-bentley disabled auto-merge June 25, 2024 23:24
@tcobbs-bentley tcobbs-bentley merged commit bbe6c55 into main Jun 25, 2024
5 checks passed
@tcobbs-bentley tcobbs-bentley deleted the travis/itwin-4.7.2 branch June 25, 2024 23:54
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.

2 participants