[No QA] [TS migration] Migrate markdownTable.js to Typescript #36256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SzymczakJ
changed the title
blazejkustra
suggested changes
Feb 12, 2024
| @@ -260,15 +263,13 @@ function markdownTable(table, options = {}) { | |||
| sizeMatrix.splice(1, 0, sizes); | |||
|
|
|||
| rowIndex = -1; | |||
| /** @type {Array<string>} */ | |||
| const lines = []; | |||
| const lines: Array<string> = []; | |||
There was a problem hiding this comment.
Suggested change
| const lines: Array<string> = []; | |
| const lines: string[] = []; |
|
|
||
| while (++rowIndex < cellMatrix.length) { | ||
| const row = cellMatrix[rowIndex]; | ||
| const sizes = sizeMatrix[rowIndex]; | ||
| columnIndex = -1; | ||
| /** @type {Array<string>} */ | ||
| const line = []; | ||
| const line: Array<string> = []; |
There was a problem hiding this comment.
Eslint should error here:
Suggested change
| const line: Array<string> = []; | |
| const line: string[] = []; |
Comment on lines
182
to
183
| const row: Array<string> = []; | ||
| const sizes: Array<number> = []; |
There was a problem hiding this comment.
Suggested change
| const row: Array<string> = []; | |
| const sizes: Array<number> = []; | |
| const row: string[] = []; | |
| const sizes: number[] = []; |
kubabutkiewicz
approved these changes
Feb 12, 2024
| @@ -1,150 +1,158 @@ | |||
| /* eslint-disable */ | |||
blazejkustra
approved these changes
Feb 13, 2024
There was a problem hiding this comment.
If we are unable to test it as this only runs after a PR is merged. Let's test it locally and copy the result of markdownTable to PR description, wdyt @SzymczakJ?
|
Because this migration requires other files to have import... except of require(..., let's wait for global migration of every require to import and module.exports to export. We already have a PR open on this. |
mollfpr
approved these changes
Feb 29, 2024
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25278 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@stitesExpensify This PR ready for your review! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
stitesExpensify
approved these changes
Feb 29, 2024
36 tasks
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.4.46-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.46-2 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #25278
PROPOSAL:
Tests
This is responsible for printing build details table after deployment and is used to make markdown out of 2d array, that's why this cannot be tested in the app. One way to test it is to write a simple js script that uses function mardownTable on a 2d array of strings. The terminal output of such test:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.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 so 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