-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Codeblocks web implementation #56064
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
Codeblocks web implementation #56064
Conversation
|
Can I get an adhoc build here? Web should be enough @mountiny |
|
friendly bump @mountiny 😄 |
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@mountiny could we get someone from the QA or C+ team to fully test this ^ ad-hoc? I've tested it on a few different devices/browsers and found no issues with it, but we want to ensure there are no regressions before merging this into the |
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
|
🚧 @mountiny has triggered a test hybrid app build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
@mountiny There're some conflicts, do we need to resolve them before testing? |
|
@BartoszGrajdek Bug on light mode
|
…ight mode backgrounds
|
|
|
@dukenv0307 the fixes are ready, you can go ahead and test them. I believe you should be able to run it locally, just make sure to reinstall node modules |
|
|
|
Added a fix to an additional problem I found, but it's still ready for testing @dukenv0307 |
|
Thank you @BartoszGrajdek, I'm testing now |
|
@BartoszGrajdek I think the fontSize here isn't correct.
|
|
|
|
@dukenv0307 Here is the fix |
|
@Skalakid looks good. Can you resolve the conflicts and open the PR?
|
|
@dukenv0307 so everything is tested and we can open the PR, right? So here is a plan of what we gonna do now:
Let me know what do you think about it :D And thank you for testing! |
|
@Skalakid Yes, that's fine |
|
@Skalakid Any updates? |
|
@dukenv0307 I had to prioritize other tasks over codeblocks. Once I'm finished I will come back to it and open all PRs, probably next week |
|
@Skalakid Any updates? |
|
No updates from me. We are finishing the current task. So once its done I will focus on code blocks :D |
|
Hello, I'm adding the final touches and fixing the last bugs in the Live Markdown code block PR. It should be ready soon. After the PR gets approved, I will merge it and open the PR that adds it to E/App |
|
Thank you @Skalakid for your update |
|
Update: The PR is still in progress because I found some critical parsing bugs, where the whole code block structure breaks. Pushed the fix for it today and started testing the solution on all platforms once again :D |
Do we need to test it on all platforms? Since the scope of this PR is for web only |
|
@dukenv0307 I think we need to test web only. By "all platforms," I meant Android and iOS web, too. I will finish the codeblock PR today and bump the Live Markdown version here so you can start testing |
|
any updates @Skalakid? |
|
@dukenv0307 I'm currently OOO and I will be back on the 2nd of July. The PR with code block implementation is mostly ready; I have addressed all review comments and am waiting for the second review. I will create the PR to E/App when I'm back |
|
@Skalakid any updates? |
|
Hello @dukenv0307, the PR is ready, we are just waiting for the last updates, and we need to finish some higher priority tasks before that. @jmusial took over the code block implementation PR, so he will come back to you with more updates about that |
|
hey @dukenv0307 yeah, I took over but currently involved in other tasks in E/App. Assumed that if this one was open a year it's a lower prio. If you need it let me know I'll bump it up my TODO list 😄 |
|
@dukenv0307 so expensify dev being down on Fri allowed me to merge the upstream PR in LM repo. Will test the new version with E/App this week and hopefully move this forward 😄 |
|
@jmusial any updates? |
|
@dukenv0307 codeblocks for web have been merged into E/App in this PR |
|
@mountiny If so, can you please process the payment because I already reviewed this PR? Thanks |








Explanation of Change
Fixed Issues
$ #39518
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop