updated onboarding project#40
Conversation
nkosimarz
left a comment
nkosimarz
left a comment
There was a problem hiding this comment.
You need to get a formatter for your VScode and also get rid of a lot of empty line
| @@ -0,0 +1,374 @@ | |||
| namespace onboardproject { | |||
|
|
|||
There was a problem hiding this comment.
I am manage to fix my format and empty line on the code
| const response = await fetch('http://localhost:2050/recordCount'); | ||
|
|
||
| let promise = new Promise((res, rej) => { | ||
| setTimeout(() => res("Now it's done!"), 50) |
There was a problem hiding this comment.
Yes, which mean my async function is going to be pause until a Promise is Settled (that is, fulfilled or rejected), and to resume execution of the async function after fulfillment( i was using it ,to test my function inside )
FritzOnFire
left a comment
FritzOnFire
left a comment
There was a problem hiding this comment.
In general, remove your try-catches. All of them seem to be in functions that are async and return promises so the calling function should be able to catch any errors.
Please put an empty line between your function definitions.
|
Also, why is your previouses an array? I think you should rather just have a number or two. |
pierrehenrinortje
left a comment
pierrehenrinortje
left a comment
There was a problem hiding this comment.
Agree with Fritz, some extra comments on my end.
| const fromId = ConvertNumber($("#index").val() as string, false); | ||
| const possibleStep = calculateToId(fromId) - fromId; | ||
| if (fromId < 0) { | ||
| alert('only insert Id greater than or equal to 0'); |
There was a problem hiding this comment.
add a return here so we can remove the next "else" below. This is more readable and reduces excessive indentation in cases where larger nested "if else" statements exist.
There was a problem hiding this comment.
The comment on this line is still relevant.
| namespace onboardproject { | ||
| module onboardprojects { | ||
|
|
||
| //Load Variable declarations |
There was a problem hiding this comment.
Please add a space after the //
There was a problem hiding this comment.
My comment on this line is still relevant.
| let Load = 50; | ||
| // Previous Variable declarations | ||
| let previous: number[]; | ||
| //Previous Process Variable declarations |
There was a problem hiding this comment.
Please add a space after the //
There was a problem hiding this comment.
My comment on this line is still relevant.
| }); | ||
| //Searching function for index | ||
| $("#go-button").click(async () => { | ||
| const recordCount = await getRecordCountCall(); |
There was a problem hiding this comment.
You are allowed to skip this step. For this project the record count will not change after the initial call.
There was a problem hiding this comment.
So the formatting looks a lot better. But for me to approve this, I am going to need you to remove all the try-catchs, and you have to stop using an array to store your fromid and toid (it just feels like an anti-pattern).
Oh, and add your dist/app.min.js to your gitignore.
| // let promise = new Promise((res, rej) => { | ||
| // setTimeout(() => res("Now it's done!"), 50) | ||
| // }); |
There was a problem hiding this comment.
I don't think you need these comments anymore.
| // let promise = new Promise((res, rej) => { | ||
| // setTimeout(() => res("Now it's done!"), 50) | ||
| // }); |
There was a problem hiding this comment.
I don't think you need these comments anymore.
| // let promise = new Promise((res, rej) => { | ||
| // setTimeout(() => res("Now it's done!"), 50) | ||
| // }); |
There was a problem hiding this comment.
I don't think you need these comments anymore.
| // } | ||
| // }; | ||
| // } | ||
| //region Data Loading methods |
There was a problem hiding this comment.
My comment is still relevant on this line.
| DisplayContent += '</tr>'; | ||
| $("#wrapper-table-content-body").empty(); | ||
| $("#wrapper-table-content-body").append(DisplayContent); | ||
| throw new Error("Error No data"); |
There was a problem hiding this comment.
Would this not throw an error every time you call this function?
| return toID; | ||
| } | ||
|
|
||
| //Onload Function |
There was a problem hiding this comment.
Please add a space after the //, and an empty line above this line.
| //Onload Function | ||
| window.onload = async () => { | ||
|
|
||
| //Previous_Page_Resize Function |
There was a problem hiding this comment.
Please add a space after the //, and an empty line above this line.
| return [previous[0] - (nextPageResize(previous) - previous[0]), toId]; | ||
| } | ||
|
|
||
| //On Resize_Function |
There was a problem hiding this comment.
Please add a space after the //, and an empty line above this line.
| const toId = calculateToId(previous[0] - (nextPageResize(previous) - previous[0])); | ||
| return [previous[0] - (nextPageResize(previous) - previous[0]), toId]; |
There was a problem hiding this comment.
You should rather store the result of nextPageResize and use that, than calling that function multiple times
| const fromId = ConvertNumber($("#index").val() as string, false); | ||
| const possibleStep = calculateToId(fromId) - fromId; | ||
| if (fromId < 0) { | ||
| alert('only insert Id greater than or equal to 0'); |
There was a problem hiding this comment.
The comment on this line is still relevant.
4 participants
No description provided.