Skip to content

Convert observable property functions to a React Hook#10838

Merged
zachmargolis merged 2 commits intomainfrom
margolis-convert-to-hook
Jun 21, 2024
Merged

Convert observable property functions to a React Hook#10838
zachmargolis merged 2 commits intomainfrom
margolis-convert-to-hook

Conversation

@zachmargolis
Copy link
Contributor

Incorporating PR feedback: #10809 (comment)

This is in draft mode because the acuant-capture-canvas-spec.jsx is failing, I think there's some out-of-order event subscription happening

changelog: Internal, Source code, Update code to match React conventions
@zachmargolis zachmargolis requested a review from aduth June 20, 2024 18:16
@zachmargolis zachmargolis changed the title Convert pair of observable property function to a React Hook Convert observable property functions to a React Hook Jun 20, 2024
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I haven't tested, but direction LGTM. Looks like there's a related build failure. (Edit: Already noted in PR description)

@zachmargolis zachmargolis marked this pull request as ready for review June 20, 2024 22:21
@zachmargolis
Copy link
Contributor Author

I haven't tested, but direction LGTM. Looks like there's a related build failure. (Edit: Already noted in PR description)

Got it working in 32f7b2a via act() in the specs, this is ready for review now!

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM!

@zachmargolis
Copy link
Contributor Author

Verified that the SDK seems to work properly by going through IDV on mobile using the review aopp

@zachmargolis zachmargolis merged commit d5ee33c into main Jun 21, 2024
@zachmargolis zachmargolis deleted the margolis-convert-to-hook branch June 21, 2024 16:11
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