-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Update live markdown to fix emoji being formatted #63306
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
Update live markdown to fix emoji being formatted #63306
Conversation
|
Hmm, on iOS, the strikethrough is still applied. |
I think we should apply this to iOS too, but I can't run the example app. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppBuild issues. NAB |
Yeah, we have an issue created before for that here, but was closed. |
Okay, let's not worry about that one then but we should address the iOS one |
Are you able to run the example app? I have tried reinstalling the pods, but I am still getting an error. |
I didn't run the example app. If you are still facing build error please post on slack. Otherwise you can also test directly on E/App |
I was changing the wrong file 🤦. I have opened another live markdown PR to fix it. |
Hey @bernhardoj @s77rt this also includes a fix for an issue we need to unblock the rn 0.79 upgrade. I added the issue and qa steps, coul dyou also test that and move this PR ahead as your top priority please? cc @war-in Let us know in slack if you have any questions. |
package.json
Outdated
@@ -84,7 +84,7 @@ | |||
"@dotlottie/react-player": "^1.6.3", | |||
"@expensify/react-native-background-task": "file:./modules/background-task", | |||
"@expensify/react-native-hybrid-app": "file:./modules/hybrid-app", | |||
"@expensify/react-native-live-markdown": "0.1.280", | |||
"@expensify/react-native-live-markdown": "0.1.285", |
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.
Update to 0.1.286
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.
That's not the version that would unblock the rn upgrade, right? If yes, then we can do that on a separate 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.
I'm not totally sure but it's the version I'm upgrading to in Mobile-Expensify and eventually we will update to that version anyway
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.
0.1.284 is the one that will unblock the RN upgrade but I agree with @s77rt that we would have to update to it anyway
However, it potentially could introduce some regressions so it'll be safer to update to 85
I'll let you decide because I don't have a strong opinion on that
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.
However, it potentially could introduce some regressions so it'll be safer to update to 85
This is my concern. We are already 41 versions ahead of main
😅
Here is the iOS video for #62902 ios.mp4 |
I'm seeing a jump on adding/removing the emoji Screen.Recording.2025-06-05.at.2.08.47.PM.mov |
The whole app feels slow on my end so the above bug may be just a consequence. Let's not block on that |
@bernhardoj Can you please retest on web? I'm getting an import error and not sure if it's just me or some other bug
|
Hmm, yeah I can repro that. |
@jmusial Could you please take a look at this error? #63306 (comment) It seems like it was introduced in Expensify/react-native-live-markdown#686 where we bumped react-native-builder-bob. |
hey @tomekzaw indeed it's this PR 😢 @s77rt @bernhardoj as a temp workaround you can replace
and after rebuild it should work ok. I'm going to make a PR with this fix ASAP |
The fix has been merged please use version: 0.1.287 |
@bernhardoj let's also update the Podfile.lock 🙏 |
Oh, you're right. Thanks for reminding! Updated. |
@mollfpr We can proceed here |
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! Thank you!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.1.63-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.1.63-6 🚀
|
Explanation of Change
Fixed Issues
$ #62029
$ #62902
PROPOSAL: #62029 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
_*~😄~*_
)PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectioncanBeMissing
param foruseOnyx
toggleReport
and notonIconClick
)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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop