-
Notifications
You must be signed in to change notification settings - Fork 76
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
Implement useOnyx hook #462
Conversation
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.
Some minor suggestions and a question, I'll test it out tomorrow!
The merge-base changed after approval.
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.
tests look great! everything looks great! I seriously considered merging this as-is but I think the last blocker for me is that we should use a constant for loading state 🙂
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.
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
useOnyx
hook.Relevant links:
Related Issues
Expensify/App#34339
Automated Tests
N/A
Manual Tests
N/A
Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop