Closed dominictb closed 4 months ago
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅
I have read the CLA Document and I hereby sign the CLA
recheck
Assigned reviewers of https://github.com/Expensify/App/issues/39852
@dominictb please complete the author checklist and perform some basic manual testing of this PR with a local build of E/App using these changes.
Will do it as soon as possible. Thanks. Feels free to review the rest of the PR
Reviewed it once, going through once again.
LGTM, pending one confirmation.
@roryabraham Do we do a checklist here? or when we bump the App PR with updated Onyx version?
@mananjadhav some videos are uploaded in the PR description (local build). Feel free to check.
Thanks @dominictb I did check those.
Do we do a checklist here?
We should do a checklist here with a build of react-native-onyx linked against E/App dev. See instructions in the README on how to do this.
uh oh, @mananjadhav anything I need to do to complete the PR? Thanks!
No. I'll finish this checklist today.
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. for onClick={this.submit}
the method this.submit
should be bound to this
in the constructor)this
are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this);
if this.submit
is never passed to a component event handler like onClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
is working as expected in all cases)Design
label and/or tagged @Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test
steps.@roryabraham All yours.
looks like reassure is just not set up correctly in Onyx?
@roryabraham I merged main. Can u check?
@roryabraham Can you action the PR so that the workflows are executed?
@roryabraham @mananjadhav let's merge this so I can bump the version in Expensify/App
@roryabraham @mananjadhav any update before we can merge?
sorry for the delay. As this is your first PR in this repo I have to manually approve workflows to run, so that's why it was delayed.
🚀Published to npm in v2.0.35
Details
Add some basic runtime type validation in Onyx for merge, set and mergeCollection operation
Related Issues
https://github.com/Expensify/App/issues/39852
Automated Tests
Test 1: For set operation: https://github.com/Expensify/react-native-onyx/blob/9ee724140cf4d78ce8d298545fa8445067a4f192/tests/unit/onyxTest.js#L75
Test 2: For merge operation: https://github.com/Expensify/react-native-onyx/blob/9ee724140cf4d78ce8d298545fa8445067a4f192/tests/unit/onyxTest.js#L117
Test 3: For
mergeCollection
: https://github.com/Expensify/react-native-onyx/blob/9ee724140cf4d78ce8d298545fa8445067a4f192/tests/unit/onyxTest.js#L434Manual 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
https://github.com/Expensify/react-native-onyx/assets/165644294/e4737868-d9c4-4b8c-af18-21bdea418e02Android: mWeb Chrome
https://github.com/Expensify/react-native-onyx/assets/165644294/c04b147e-6a0f-496d-a18b-b08481b31588iOS: Native
https://github.com/Expensify/react-native-onyx/assets/165644294/a66c1482-1055-462c-874d-04052165d646iOS: mWeb Safari
https://github.com/Expensify/react-native-onyx/assets/165644294/177737b9-d955-4080-9242-5736bb248995MacOS: Chrome / Safari
https://github.com/Expensify/react-native-onyx/assets/165644294/31101ff4-ebb1-4c66-8441-41f31b414e18MacOS: Desktop
https://github.com/Expensify/react-native-onyx/assets/165644294/efd979e6-4840-4796-860b-77b94afad16c