-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add Live Markdown refactor changes #45150
Add Live Markdown refactor changes #45150
Conversation
@dubielzyk-expensify @Ollyws One of you needs to 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] |
This PR contains only temporary changes, added only for QA team regression tests. After this PR pass all the tests we will merge the refactor PR to library and than bump the version in this PR to the latest |
This comment has been minimized.
This comment has been minimized.
…id/live-markdown-for-web-refactor
434c4fa
to
7af6e67
Compare
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Requested the QA for this |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Chat - Console error when selecting emoji from suggestion listVersion Number: 9.0.6-1 Action Performed:
Expected Result:No console error will show up. Actual Result:Console error shows up. Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6539655_1720756941589.bandicam_2024-07-12_11-52-04-691.mp4Bug6539655_1720756941618!45150.pr-testing.expensify.com-1720756341776.txt Upwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Chat - Text pasted into the composer doubles if the user returns from a threadVersion Number: 9.0.6-2 Action Performed:
Expected Result:The copied text should be pasted into the composer. Actual Result:Text pasted into the composer doubles if the user returns from a thread. Affects all chats after the issue is triggered. Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6539658_1720757397506.bandicam_2024-07-12_05-57-32-630.mp4Upwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Chat - Line breaks are added after each line to the text pasted from a code editorVersion Number: 9.0.6-2 Action Performed:
Expected Result:The text in the composer should look like in the code editor. Actual Result:Line breaks are added after each line to the text pasted to the composer from a code editor Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6539665_1720758073198.bandicam_2024-07-12_06-16-19-296.mp4Upwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! "Nothing to show" popup is over appearing over "Skeleton"Version Number: 9.0.6-1 Action Performed:PreCond: User has no drafts
Expected Result:User expects to see the "nothing to show" popup with a blank background. User does not expect a Skeleton WITH a popup as well. Actual Result:There is a "Skeleton" with the popup showing over it Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosUpwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Task/IOU - Unable to paste content to description fieldVersion Number: 9.0.6-1 Action Performed:
Expected Result:The copied content is pasted in the description field and displayed Actual Result:The copied content is not pasted and not displayed Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6539672_1720758981416.Recording__592.mp4Upwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Expense - Empty line is created before and after the text in the code block in descriptionVersion Number: 9.0.6-1 Action Performed:
Expected Result:There will be no changes to the code block in the description. Actual Result:Empty line is created before and after the text in the code block in expense description. Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6539676_1720759711940.bandicam_2024-07-12_12-44-39-410.mp4Upwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Expense - Cursor moves to the front of emoji after selecting emoji in description fieldVersion Number: 9.0.6-1 Action Performed:Precondition:
Expected Result:The cursor will move to the end of the description content. Actual Result:The cursor moves to the front of the emoji after selecting an emoji. Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6539699_1720763160370.bandicam_2024-07-12_13-38-53-755.mp4Recording.2620.mp4Upwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Composer - Cursor moves in front of emoji if emoji inserted by using :emoji:Version Number: 9.0.6-1 Action Performed:Precondition:
Expected Result:The cursor is displayed after the emoji Actual Result:The cursor moves in front of the emoji Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6539682_1720761187759.video_2024-07-12_01-12-42.mp4Upwork Automation - Do Not Edit |
@mountiny @IuliiaHerets All reported bugs should be fixed now and we got rid of all critical bugs connected to refactored architecture and logic of Live Markdown for web. How about moving forward with refactor and merging the changes in |
@mountiny can you run the build last time, so we can confirm that everything has been fixed, please? |
Regression is completed |
@mountiny @Ollyws @dubielzyk-expensify @thienlnam The Live Markdown refactor has been finally merged inside the library! Because of this, I removed temporary changes made for QA testing purposes and bumped the version of |
👍 Will review later today or tomorrow |
@Ollyws Can you please tag me in slack when you are done |
The cut button doesn't seem to be working for me on iOS Safari, however it's working fine on main: cut_not_working.mov |
@Ollyws Are you able to put this to the top of your priorities? Just so we can get it merged as soon as possible, thank you! |
Yeah sure, I'm reviewing at the moment. |
Another issue I'm getting is on Android Chrome the selection is immediately being lost: androidselectionproblem.mov |
@Ollyws I can't reproduce the second bug on Android Chrome :/ Can you please delete node modules, run Screen.Recording.2024-08-22.at.14.03.46.mov |
Ah it seems to be specific to that emulator and API I was using so I guess it's an issue with the emulator itself, we can forget about that one. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeN/A Android: mWeb Chrome02_Android_Chrome.mp4iOS: NativeN/A iOS: mWeb Safari04_iOS_Safari.mp4MacOS: Chrome / Safari05_MacOS_Chrome.mp4MacOS: Desktop06_MacOS_Desktop.mp4 |
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.
Aside from #45150 (comment) which is being dealt with in Expensify/react-native-live-markdown#459, the rest LGTM.
Let's do it!!! |
✋ 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/thienlnam in version: 9.0.25-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/thienlnam in version: 9.0.25-0 🚀
|
cc: @mountiny
Details
This PR contains bump to the new version of
react-native-live-markdown
from Expensify/react-native-live-markdown#394. The changes in this PR are temporary and were added only for QA testing purposes. The bump was added as apackage.tgz
file because of some problems with linking the branch/commit to package.json.In this PR to Expensify, we want to check huge structure and logic changes in Live Markdown for the web before merging them in the library and enabling them inside the app. For more specific info please visit original PR.
The refactor was made as a part of the Inline Image feature. We needed to change the current Live Markdown Input logic and structure to enable displaying images inside the input. Also, we decided to enhance the current library implementation to:
Fixed Issues
Helps with #40181
$ #48581
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Refactor was made only for web/mWeb/desktop platforms, please test only these platforms.
Since it's a big change it would be great to test it in all cases connected to:
Test steps from #47763 (version 0.1.116):
Verify that: When hitting the enter key after a mark down the new line becomes visible with the cursor seen visibly
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop