Conversation
blazejkustra
left a comment
There was a problem hiding this comment.
Some minor suggestions and a question, I'll test it out tomorrow!
The merge-base changed after approval.
blazejkustra
left a comment
There was a problem hiding this comment.
LGTM! 🚀
I tested it in the App today, also migrated some withOnyx usage to check if it works as expected, looks very promising! Great work @fabioh8010
|
Does there need to be anything generated for the API docs for this? |
|
@tgolen I've ran the script to update the docs |
|
LGTM 🚀 |
|
As noted by others, the code changes are looking great! Not leaving a review so I don't take credits for weeks worth of work, but wanted to note that you have done great job with this project, not only Fabio and Blazej, but also Rory and Tim (and others leaving a review on the doc) for pushing this forwards, thank you! 🏅 |
Details
This PR adds a new way to consume Onyx data in the application, the
useOnyxhook.Relevant links:
Related Issues
Expensify/App#34339
Automated Tests
N/A
Manual Tests
N/A
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop