-
Notifications
You must be signed in to change notification settings - Fork 71
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
Pass withOnyx instance props to selectors #247
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.
LGTM, left a single comment but NAB IMO
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!
OK, I found with @grgia that we needed to pass the state instead of props, and I also found a spot where I wasn't passing the parameter to the selector properly. I've updated tests and docs |
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.
havent tested, LGTM. this will be a nice addition.
What's left to merge this one? |
@tgolen I wasn't able to get it working on my branch using the selector / props - HOC is here if you want to take a look - Expensify/App#18638 |
I'm not sure it's working as expected, I only seem to get |
One way I tested this to ensure it was working was to edit
|
Since this is working with the unit tests and in the real-world, I'm going to go ahead and merge this. Maybe one day I will get the chance to implement it in NewDot |
Details
This is needed for threads. See the discussion here
Fixes
$ Expensify/App#18489
Automated Tests
Everything is covered by tests
Linked PRs
@grgia will be implementing this