-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix app crashes when edit the video with text #46693
Conversation
@ZhenjaHorbach Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: Native2024-08-02.10.50.51.movAndroid: mWeb Chromeweb-android.moviOS: Nativeios.moviOS: mWeb Safariweb-ios.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
@nkdengineer But I noticed that in native apps LHN items are displayed a little incorrectly (there is no [attachment]) Can you check, please ? |
@ZhenjaHorbach Is this reproducible on the latest main? |
@ZhenjaHorbach I also can reproduce on the latest main and the staging, I think we should handle this as a separate issue since it's out of the scope and we can avoid regression. |
I agree In this case LGTM ! |
@@ -1871,7 +1871,7 @@ PODS: | |||
- RNGoogleSignin (10.0.1): | |||
- GoogleSignIn (~> 7.0) | |||
- React-Core | |||
- RNLiveMarkdown (0.1.107): | |||
- RNLiveMarkdown (0.1.111): |
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.
What were the other PRs merged in this diff? have we tested the App for all of them?
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.
We only have another PR here Expensify/react-native-live-markdown#374.
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.
@BartoszGrajdek I'm planning to bump the react-native-live-markdown
version here. Can you please confirm this PR Expensify/react-native-live-markdown#374 works well in E/App now?
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.
@BartoszGrajdek
Friendly bump !
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.
Actually, @Skalakid was the reviewer also
Can you please confirm this PR Expensify/react-native-live-markdown#374 works well in E/App now?
Because we are planning to bump the react-native-live-markdown version in this PR
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.
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.
Asked in internal slack channel with swm https://expensify.slack.com/archives/C06BDSWLDPB/p1723155420752799
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.
Should be ok to upgrade
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
FYI I believe this was deployed to prod yesterday, from this checklist - #47219 |
Details
Fix app crashes when edit the video with text
Fixed Issues
$ #46114
PROPOSAL: #46114 (comment)
Tests
Offline tests
Same as above
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
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 theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-08-02.at.12.32.07.mov
Android: mWeb Chrome
Screen.Recording.2024-08-02.at.11.54.32.mov
iOS: Native
Screen.Recording.2024-08-02.at.11.59.05.mov
iOS: mWeb Safari
Screen.Recording.2024-08-02.at.11.58.38.mov
MacOS: Chrome / Safari
Screen.Recording.2024-08-02.at.11.51.47.mov
MacOS: Desktop
Screen.Recording.2024-08-02.at.12.03.06.mov