updated onboarding project by SAMMOLOI · Pull Request #40 · IMQS/onboard-javascript

SAMMOLOI · 2021-08-19T13:54:03Z

No description provided.

nkosimarz

You need to get a formatter for your VScode and also get rid of a lot of empty line

nkosimarz · 2021-08-20T06:59:10Z

app.ts

@@ -0,0 +1,374 @@
+namespace onboardproject {
+


remove empty lines

I am manage to fix my format and empty line on the code

nkosimarz · 2021-08-20T07:01:03Z

app.ts

+        const response = await fetch('http://localhost:2050/recordCount');
+
+        let promise = new Promise((res, rej) => {
+            setTimeout(() => res("Now it's done!"), 50)


is this meant to be Load

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 )

app.ts

FritzOnFire

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.

app.ts

index.html

style.css

FritzOnFire · 2021-08-23T10:24:46Z

Also, why is your previouses an array? I think you should rather just have a number or two.

pierrehenrinortje

Agree with Fritz, some extra comments on my end.

app.ts

pierrehenrinortje · 2021-08-23T12:26:02Z

app.ts

+				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');


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.

The comment on this line is still relevant.

app.ts

FritzOnFire · 2021-08-23T15:42:31Z

app.ts

+namespace onboardproject {
+	module onboardprojects {
+
+		//Load Variable declarations


Please add a space after the //

My comment on this line is still relevant.

FritzOnFire · 2021-08-23T15:42:35Z

app.ts

+		let Load = 50;
+		// Previous Variable declarations
+		let previous: number[];
+		//Previous Process Variable declarations


Please add a space after the //

My comment on this line is still relevant.

app.ts

FritzOnFire · 2021-08-23T15:59:58Z

app.ts

+			});
+			//Searching function for index
+			$("#go-button").click(async () => {
+				const recordCount = await getRecordCountCall();


You are allowed to skip this step. For this project the record count will not change after the initial call.

FritzOnFire

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.

FritzOnFire · 2021-08-25T09:21:36Z

app.ts

+			// let promise = new Promise((res, rej) => {
+			// 	setTimeout(() => res("Now it's done!"), 50)
+			// });


I don't think you need these comments anymore.

FritzOnFire · 2021-08-25T09:22:17Z

app.ts

+			// let promise = new Promise((res, rej) => {
+			// 	setTimeout(() => res("Now it's done!"), 50)
+			// });


I don't think you need these comments anymore.

FritzOnFire · 2021-08-25T09:22:33Z

app.ts

+			// let promise = new Promise((res, rej) => {
+			// 	setTimeout(() => res("Now it's done!"), 50)
+			// });


I don't think you need these comments anymore.

FritzOnFire · 2021-08-25T09:23:23Z

app.ts

+		//     }
+		// };
+		// }
+		//region Data Loading methods


My comment is still relevant on this line.

FritzOnFire · 2021-08-25T09:24:31Z

app.ts

+				DisplayContent += '</tr>';
+				$("#wrapper-table-content-body").empty();
+				$("#wrapper-table-content-body").append(DisplayContent);
+				throw new Error("Error No data");


Would this not throw an error every time you call this function?

FritzOnFire · 2021-08-25T09:38:32Z

app.ts

+			return toID;
+		}
+
+		//Onload Function


Please add a space after the //, and an empty line above this line.

FritzOnFire · 2021-08-25T09:38:35Z

app.ts

+		//Onload Function
+		window.onload = async () => {
+
+			//Previous_Page_Resize Function


Please add a space after the //, and an empty line above this line.

FritzOnFire · 2021-08-25T09:38:39Z

app.ts

+				return [previous[0] - (nextPageResize(previous) - previous[0]), toId];
+			}
+
+			//On Resize_Function


Please add a space after the //, and an empty line above this line.

FritzOnFire · 2021-08-25T09:41:11Z

app.ts

+				const toId = calculateToId(previous[0] - (nextPageResize(previous) - previous[0]));
+				return [previous[0] - (nextPageResize(previous) - previous[0]), toId];


You should rather store the result of nextPageResize and use that, than calling that function multiple times

FritzOnFire · 2021-08-25T09:43:48Z

app.ts

+				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');


The comment on this line is still relevant.

Fixed repo clone

62eee76

SAMMOLOI requested review from FritzOnFire, geraldtivatyi, nkosimarz and pierrehenrinortje August 19, 2021 13:54

nkosimarz suggested changes Aug 20, 2021

View reviewed changes

SAMMOLOI added 3 commits August 20, 2021 13:37

update

fd00445

update

4731cd9

update

c7cac80

SAMMOLOI requested a review from nkosimarz August 23, 2021 09:38

FritzOnFire requested changes Aug 23, 2021

View reviewed changes

SAMMOLOI added 3 commits August 23, 2021 13:43

update

eeb8e54

update

c652401

update

031b614

pierrehenrinortje suggested changes Aug 23, 2021

View reviewed changes

SAMMOLOI added 4 commits August 23, 2021 16:24

update

027b329

update

909ce8f

update

1de2f8d

update

fa95818

FritzOnFire requested changes Aug 23, 2021

View reviewed changes

SAMMOLOI added 9 commits August 24, 2021 08:36

update

0e43a23

update

b2e0238

update

32a64cc

update

4f10567

update

97d4868

update

98cbe57

update

2672789

update

5aaf23d

update

89e549c

SAMMOLOI added 9 commits August 24, 2021 15:28

update

1baa7d2

update

b50d91d

update

a1a101a

update

17a729f

update

980ab3f

update

aa08fe6

update

07be5f0

update

f02c455

update

53b603d

SAMMOLOI closed this Aug 25, 2021

SAMMOLOI reopened this Aug 25, 2021

SAMMOLOI added 2 commits August 25, 2021 10:52

update

39faae7

update

6082bc3

FritzOnFire requested changes Aug 25, 2021

View reviewed changes

SAMMOLOI added 16 commits August 25, 2021 11:51

update

88d921b

update

a329804

update

7939c12

update

7591502

update

bcc9696

update

f7fe9a7

update

44aad0a

update

5ee895e

update

14fbe67

update

6b25495

update

ae33eae

update

19d0292

update

6aa32a9

update

5efc63c

update

8aab829

update

b25c082

		const toId = calculateToId(previous[0] - (nextPageResize(previous) - previous[0]));
		return [previous[0] - (nextPageResize(previous) - previous[0]), toId];

Conversation

SAMMOLOI commented Aug 19, 2021

Uh oh!

nkosimarz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FritzOnFire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FritzOnFire commented Aug 23, 2021

Uh oh!

pierrehenrinortje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FritzOnFire left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

FritzOnFire left a comment •

edited

Loading