Closed jnowakow closed 3 months ago
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅
I have read the CLA Document and I hereby sign the CLA
### 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.Yep, I'm on it!
How you getting on @allroundexperts?
@trjExpensify I was unable to verify the fix. For me, the table values do not get updated.
@allroundexperts just confirming that you updated App to use this branch of Onyx?
@luacmartins Yes, I checked out this branch and built it using npm run build
. After that, I copied the dist
folder and replaced it inside the node_modules
of the main app.
@Kicu Any update on above?
@allroundexperts I'm investigating it. It's strange because it seems that there is problem with updating component since data in onyx are updated:
I checkout out also version before optimisation and it works the same. I'll dig into it further
@allroundexperts problem lies in App. Two days ago this PR was merged. It wraps all components in Search table in memo
. The problem is that function passed to memo
checks only keyForList
. So even if amount
changes the component isn't re-rendered and shows outdated value.
For testing purposes you can modify TotalCell
component like this:
const TotalCell = memo(({showTooltip, amount, currency, isLargeScreenWidth}: CurrencyCellProps) => {
const styles = useThemeStyles();
return (
<TextWithTooltip
shouldShowTooltip={!!showTooltip}
text={CurrencyUtils.convertToDisplayString(amount, currency)}
style={[styles.optionDisplayName, styles.textNewKansasNormal, styles.pre, styles.justifyContentCenter, isLargeScreenWidth ? undefined : styles.textAlignRight]}
/>
);
});
It's defined in src/components/SelectionList/Search/TransactionListItemRow.tsx
file.
Go-to solution is to modify functions passed to memo
because other cells won't update if they props change
cc @luacmartins
Nice find @jnowakow! Yea, let's update the prop comparison function to handle these updates too!
@luacmartins sure thing! I'll work on that. Should I create issue for this?
@jnowakow created issue here
@allroundexperts were you able to test following @jnowakow's instructions here?
Not really. Isn't this on hold?
I think just a merge hold. We can still continue development. What part of the instructions didn't work?
Oh, I didn't continue the testing after hold was applied. Will re-test and get back today.
Just tested this again. The new search page seems to be broken now 😞
https://github.com/Expensify/react-native-onyx/assets/30054992/3885ab45-862d-4675-8cb9-f58085d0d9dd
@allroundexperts yea, we messed up the API a bit. To temporarily fix it you can change this value to transactions
instead of transaction
@luacmartins It works now. But on Android, I'm getting the following crash:
https://github.com/Expensify/react-native-onyx/assets/30054992/3e4eeb58-0ca9-4edb-ba30-1ae35a24d50d
@allroundexperts that seems unrelated to this PR. Can we focus testing this Onyx changes in Web first? We should be able to see the live updates in Search when they are edited.
@allroundexperts that seems unrelated to this PR. Can we focus testing this Onyx changes in Web first? We should be able to see the live updates in Search when they are edited.
I can confirm that for web, it is working fine.
Working on the checklist now.
The holding PR was merged, we can remove the hold here
🚀Published to npm in v2.0.49
Details
This PR adds snapshot functionality described in design doc. It adds special support for
SNAPSHOT
key. Objects inside itsdata
field will be updated with every change made throughupdate
function.Related Issues
https://github.com/Expensify/App/issues/41626
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
https://github.com/Expensify/react-native-onyx/assets/56261019/10a934db-4d85-4f45-9b16-4b96ad51dc8bAndroid: mWeb Chrome
https://github.com/Expensify/react-native-onyx/assets/56261019/f62863e7-a3a1-466d-b686-8cfa635a00f5iOS: Native
https://github.com/Expensify/react-native-onyx/assets/56261019/ee8e10f2-1a7b-4e61-b5d7-446204887de2iOS: mWeb Safari
https://github.com/Expensify/react-native-onyx/assets/56261019/4d3ecc3f-8ac6-4b3f-8084-0614e9d4b850MacOS: Chrome / Safari
https://github.com/Expensify/react-native-onyx/assets/56261019/dc1c3af3-e6cf-47ce-832c-ea5bb18d2561MacOS: Desktop
https://github.com/Expensify/react-native-onyx/assets/56261019/fc51d7e0-1730-4b71-a171-762c50d3b300