-
Notifications
You must be signed in to change notification settings - Fork 80
Implemented partialSetCollection #677
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
base: main
Are you sure you want to change the base?
Implemented partialSetCollection #677
Conversation
@shubham1206agra is this implementation directly coming from the design doc, or is there any linked conversation which I can follow? |
@parasharrajat We discussed about this here https://expensify.slack.com/archives/C08CZDJFJ77/p1755275332300729 |
Hi, what's the status of this PR and what are the next steps? |
@parasharrajat You can start the testing now. Just test for any anomalies in the App. |
@shubham1206agra There is a conflict in Onyx that will probably effect this PR that you want to fix before having it tested. |
@tgolen Done |
Thanks! @parasharrajat you can start testing whenever you are ready then! |
Sure, |
Details
Implemented
partialSetCollection
so that update uses this for collection keys to eliminate extra notify calls.Related Issues
GH_LINK
Automated Tests
Manual Tests
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