Onboarding Project: Maxwill #48

MaxTF141 · 2023-08-14T07:09:01Z

No description provided.

…This time by ID.

…Removed my global search function and inserted functionality to only search by id.

…t. Updated pagination so it can work with the adjustDisplayedRecords function.

…of records displayed. It depends on the screen height and the page.

… same.

…HEAD

…th page rows. App doesn't crash when resizing too low.

…rs. Finished with all the functionalities.

DevinFledermaus · 2023-08-28T09:41:25Z

app.ts

+let lastPage = firstPage + 9
+let currentFromID = 1;
+let currentToID = 20;
+let difference = 0


Try to only make use of 2 global variables and give them a type

No need to give the Type if you set the variable

We should not have any global variables.

And you only have to set the type if they where class variables or if they were normal variables that were not assigned a value in the same line. And these variables are neither.

DevinFledermaus · 2023-08-28T09:41:52Z

app.ts

+let difference = 0
+
+
+// This function  will handle retrieving the records from the api


no need for so much space between the code, we all love each other here

DevinFledermaus · 2023-08-28T09:47:35Z

app.ts

+async function getColumns(): Promise<Array<string>> {
+	try {
+		const data = await fetch(`${api}columns`);
+		const columns: Array<string> = await data.json();


Suggested change

const columns: Array<string> = await data.json();

const columns: string[]= JSON.parse(data);

Always make use of .json() instead of JSON.parse() when able. But the string[] is still the correct comment.

DevinFledermaus · 2023-08-28T09:47:50Z

app.ts

+		return columns;
+
+	} catch (error) {
+		console.error(error);


no need to console log

DevinFledermaus · 2023-08-28T10:00:45Z

app.ts

+	firstPage = lastPage - 9;
+	$(".pagination").empty();
+	await pageNumbers(firstPage, lastPage);
+	// isFunctionRunning = false


did you mean to comment this out, looks like it could break if you press prev and then next

DevinFledermaus · 2023-08-28T10:04:33Z

app.ts

+		}
+		if (firstPage === 1) {
+			$(".prev").css({ display: "none" });
+		} else $(".prev").css({ display: "block" });


no {} for else?

DevinFledermaus

Lots of you functions make use of the same variables that you keep having to set, maybe get them all in a central place. will make lots of your functions way smaller

FritzOnFire · 2023-09-11T07:10:02Z

apiManager.js

Please don't add your .js files to this PR.

FritzOnFire · 2023-09-11T07:10:19Z

apiManager.js.map

Please don't add your .js.map files to this PR.

FritzOnFire · 2023-09-11T07:49:30Z

apiManager.ts

+	private mainUrl: string;
+	constructor(mainUrl: string) {


Please leave an empty line between your class variables and constructor

FritzOnFire · 2023-09-11T07:53:23Z

apiManager.ts

+		return fetch(mainUrl)
+			.then(res => res.text())
+			.then(data => JSON.parse(data))
+			.catch((err) => {
+				throw err
+			})


You need to check if your fetch was successful, as that does not get catch'd by the .catch.

Please use res.json() instead of res.text() and then JSON.parse(). (I know someone else asked you to do it this way, and I apologize that you have to revert your code now again, but this is the right way)

Also, please add a ; to the throw and at the end.

FritzOnFire · 2023-09-11T07:54:45Z

apiManager.ts

+		return fetch(mainUrl)
+			.then(res => res.text())
+			.then(data => JSON.parse(data))
+			.catch((err) => {


Either remove the () around err or specify a type

FritzOnFire · 2023-09-11T10:43:03Z

style.css

+    text-overflow: ellipsis;
+    white-space: nowrap;


FritzOnFire · 2023-09-11T10:43:34Z

style.css

+    padding-left: 10px;
+}
+
+input:focus{


Please add a space before the {

FritzOnFire · 2023-09-11T10:44:29Z

style.css

+    margin: 3px;
+    background-color: #ffffff36;
+    box-shadow: inset 0px 0px 20px -10px rgba(255, 255, 255, 0.438);
+


Please remove this one empty line.

FritzOnFire · 2023-09-11T10:45:13Z

style.css

+.header {
+    height: 5vh;
+}
+.heading {
+    height: 5vh;
+}
+.table-container {
+    height: 80vh;
+}
+.wrapper {
+    height: 10vh;
+    display: flex;
+    align-items: center;
+}


Please leave an empty line between classes.

FritzOnFire · 2023-09-11T10:46:42Z

style.css

+  @keyframes rot {
+    100% {
+      transform: rotate(360deg);
+


Please remove this one empty line.

Deleted the js files

…s. Fixed bugs that poped up after changing starting id.

anzelIMQS

I gave a very partial review. Will get back to you if necessary.

anzelIMQS · 2023-09-18T09:24:43Z

app.ts

+				}
+			})
+			.catch(error => {
+				throw new Error(`Failed to retrieve columns: ${error}`)


Suggested change

throw new Error(`Failed to retrieve columns: ${error}`)

throw new Error(`Failed to retrieve columns: ${error}`);

Missing semi-colon

anzelIMQS · 2023-09-18T09:26:06Z

app.ts

+				}
+				else if (this.currentPage === 1) {


Suggested change

}

else if (this.currentPage === 1) {

} else if (this.currentPage === 1) {

On the same line please

anzelIMQS · 2023-09-18T09:30:16Z

app.ts

+							this.paginationEnd = pageEnd;
+						}
+					} else {
+						throw new Error("Please provide a valid integer.")


Suggested change

throw new Error("Please provide a valid integer.")

throw new Error("Please provide a valid integer.");

anzelIMQS · 2023-09-18T10:46:38Z

app.ts

+			return this.apiManager.getRecordCount()
+				.then(count => {
+					let inputNumber = this.input();
+					let inputNumberInt: any = parseInt(inputNumber);


Is it always a number being returned?

anzelIMQS · 2023-09-18T13:47:05Z

apiManager.ts

+
+	/** Retrieves the number of records there are */
+	getRecordCount(): Promise<number> {
+		let test = `${this.mainUrl}recordCount`


Please remove stray

anzelIMQS · 2023-09-18T13:50:02Z

apiManager.ts

+	private fetchJson(mainUrl: string): Promise<any> {
+		return fetch(mainUrl)


Suggested change

private fetchJson(mainUrl: string): Promise<any> {

return fetch(mainUrl)

private fetchJson(url: string): Promise<any> {

return fetch(url)

Feels incorrect calling it the main one since in this scope it is only one and can be anything. Main or not.

anzelIMQS · 2023-09-18T13:50:14Z

apiManager.ts

+class ApiManager {
+	private mainUrl: string;


Space inbetween.

anzelIMQS · 2023-09-18T13:55:35Z

app.ts

+					}
+					$("#records-table-body").append(`</tr>`);
+				}
+				this.isFunctionRunning = false;


Do you know that there is a chance that your request doesn't get this far since it broke somewhere above. That would mean your isFunctionRunning will forever be true and causing no new requests to come in for showRecords.

Ideally, make use of the finally-block instead. It will always run irrespective of the request failing.

anzelIMQS · 2023-09-18T13:58:19Z

app.ts

+		let screenHeight = <number>($(window).height());
+		if (screenHeight < 68) {
+			screenHeight = 68;
+		}


Eish, then why not have a min-height prop on your window?

anzelIMQS

Please try to see how you can make use of more shared code. I still see some repetitiveness. Try to reduce it by seeing a pattern in your code.

anzelIMQS · 2023-09-20T13:34:13Z

app.ts

+	}
+
+	/** Fetching the records and rendering it on the DOM in the table*/
+	showRecords(fromID: number, toID: number): Promise<any> {


Suggested change

showRecords(fromID: number, toID: number): Promise<any> {

showRecords(fromID: number, toID: number): Promise<void> {

Change back to void

anzelIMQS · 2023-09-20T13:56:54Z

app.ts

+
+	constructor() {
+		this.isFunctionRunning = false;
+		this.apiManager = new ApiManager("http://localhost:2050/");


Suggested change

this.apiManager = new ApiManager("http://localhost:2050/");

this.apiManager = new ApiManager("http://localhost:2050");

anzelIMQS · 2023-09-20T13:57:06Z

apiManager.ts

+
+	/** Retrieves records from the api */
+	getRecords(fromID: number, toID: number): Promise<string[][]> {
+		return this.fetchJson(`${this.mainUrl}records?from=${fromID}&to=${toID}`);


Suggested change

return this.fetchJson(`${this.mainUrl}records?from=${fromID}&to=${toID}`);

return this.fetchJson(`${this.mainUrl}/records?from=${fromID}&to=${toID}`);

anzelIMQS · 2023-09-20T13:57:15Z

apiManager.ts

+
+	/** Retrieves columns from the api */
+	getColumns(): Promise<string[]> {
+		return this.fetchJson(`${this.mainUrl}columns`);


Suggested change

return this.fetchJson(`${this.mainUrl}columns`);

return this.fetchJson(`${this.mainUrl}/columns`);

anzelIMQS · 2023-09-20T13:57:23Z

apiManager.ts

+
+	/** Retrieves the number of records there are */
+	getRecordCount(): Promise<number> {
+		return this.fetchJson(`${this.mainUrl}recordCount`);


Suggested change

return this.fetchJson(`${this.mainUrl}recordCount`);

return this.fetchJson(`${this.mainUrl}/recordCount`);

anzelIMQS · 2023-09-20T15:31:18Z

app.ts

+		$(".prev").on("click", () => {
+			this.paginationEnd = this.paginationStart - 1;
+			this.paginationStart = this.paginationEnd - 9;
+			$(".pagination").empty();


You are already emptying in pageNumbers.
Please remove.

anzelIMQS · 2023-09-20T15:32:41Z

app.ts

+							start = end - maxRecords;
+						}
+						this.currentPage = Math.floor(end / maxRecords);
+						if (inputNumberInt < 1000000 && inputNumberInt > 0) {


Suggested change

if (inputNumberInt < 1000000 && inputNumberInt > 0) {

if (inputNumberInt < 1000000 && inputNumberInt > 0) {

You have your count. You went all the way to fetch it, use it and not 1000000.

anzelIMQS · 2023-09-20T15:41:02Z

app.ts

+	/** Fetching the records and rendering it on the DOM in the table*/
+	showRecords(fromID: number, toID: number): Promise<any> {
+		if (this.isFunctionRunning) {
+			return Promise.resolve(undefined);


Suggested change

return Promise.resolve(undefined);

return new Promise<void>(() => { });

anzelIMQS · 2023-09-20T15:44:58Z

app.ts

+		// element's attribute value. 
+		$(".pagination").on("click", ".pagination-item", (event) => {
+			$('.pagination-item').prop('disabled', true);
+			return this.apiManager.getRecordCount()


You don't have to return here. Just run it.

Suggested change

return this.apiManager.getRecordCount()

this.apiManager.getRecordCount()

anzelIMQS · 2023-09-20T15:46:09Z

app.ts

+					$(".pagination-item").removeClass("active");
+					$(event.target).addClass("active");
+					const self = this;
+					$(".pagination-item").each(function () {


Any reason why this can't be an arrow function?

FritzOnFire · 2023-09-26T09:56:02Z

app.ts

@@ -0,0 +1,408 @@
+class DataHandler {
+	/** It tracks if the showRecords function is running and would the value would be false if it's not. */


Suggested change

/** It tracks if the showRecords function is running and would the value would be false if it's not. */

/** It tracks if the showRecords function is running. Is false if it's not. */

FritzOnFire · 2023-09-26T09:58:09Z

apiManager.ts

+		return fetch(url)
+			.then(res => {
+				if (res.ok) {
+					return res.json()


Missing ;

FritzOnFire · 2023-09-26T09:58:12Z

apiManager.ts

+				if (res.ok) {
+					return res.json()
+				} else {
+					throw new Error(`HTTP error! Status: ${res.status}`)


Missing ;

FritzOnFire · 2023-09-26T09:59:07Z

apiManager.ts

+	getRecordCount(): Promise<number> {
+		return this.fetchJson(`${this.mainUrl}/recordCount`);
+	}


So /recordCount returns a number as text... not a json... this feels misleading. You should probably do a different fetch to handle this case.

FritzOnFire · 2023-09-26T10:00:51Z

app.ts

+class DataHandler {
+	/** It tracks if the showRecords function is running and would the value would be false if it's not. */
+	private isFunctionRunning: boolean;
+	/** This is an instance of the ApiManager class and passing a string when being used. */


You probably mean that you provide the url to the server during initialization. The way you have it written here it sounds like every function on this class takes a string as a parameter.

FritzOnFire · 2023-09-27T11:18:39Z

app.ts

+					}
+					this.currentToID = this.currentFromID + maxRecords;
+					this.currentPage = newCurrentPage;
+					let originalID = ((this.currentPage * maxRecords) + (this.currentPage - 1)) - maxRecords;


Please just change this to be

(this.currentPage - 1) * (maxRecords + 1)

It just makes it a bit clearer that you are calculating the fromID of the previous page.

FritzOnFire · 2023-09-27T11:24:17Z

app.ts

+					this.currentFromID = (this.currentPage - 1) * maxRecords + 1;
+				}
+				if (this.currentFromID === 0) {
+					let divisor = screenHeight < 136 ? maxRecords : maxRecords - 1;


I don't like this hard coded value... 136. You definitely should not need it. Something must be wrong with the way you calculate maxRecords if this is necessary. Please remove it.

FritzOnFire · 2023-09-27T11:27:47Z

app.ts

+			clearTimeout(this.resizeTimer);
+		}
+		this.resizeTimer = setTimeout(() => {
+			return this.adjustDisplayedRecords()


Returning here does not make a lot of sens. setTimeout can't do anything with your promise.

app.ts

FritzOnFire

Please only handle errors as late as possible

FritzOnFire · 2023-09-29T10:40:20Z

apiManager.ts

+		.then(res => {
+			if(res.ok) {
+				return res.text();
+			} else {
+				throw new Error(`HTTP error? Status: ${res.status}`);
+			}
+		})
+		.then (recordCount => {
+			return parseInt(recordCount);
+		})
+		.catch(error => {
+			throw error;
+		});


This is missing some indentation.

FritzOnFire · 2023-09-29T10:40:28Z

apiManager.ts

+	getRecordCount(): Promise<number> {
+		return fetch(`${this.mainUrl}/recordCount`)
+		.then(res => {
+			if(res.ok) {


Suggested change

if(res.ok) {

if (res.ok) {

FritzOnFire · 2023-09-29T10:40:39Z

apiManager.ts

+				throw new Error(`HTTP error? Status: ${res.status}`);
+			}
+		})
+		.then (recordCount => {


Suggested change

.then (recordCount => {

.then(recordCount => {

FritzOnFire · 2023-09-29T10:41:28Z

apiManager.ts

+			}
+		})
+		.then (recordCount => {
+			return parseInt(recordCount);


Would prefer it you checked for NaN here... but for this project it is probably fine.

FritzOnFire · 2023-09-29T10:47:51Z

app.ts

+					alert("An error occurred while trying to load the page. Please try again.");
+					throw error;


???

Why are you handling the error and then throwing the error anyway?

In this scenario there is no one to catch that error, which is bad, you should remove the throw.

FritzOnFire · 2023-09-29T10:53:49Z

app.ts

+		// on to the DOM.
+		$(".search-input").on("input", (e: any) => {
+			e.preventDefault();
+			return this.getRecordCount()


Pretty sure the handler function jquery is looking for does not return anything. Please remove the return.

FritzOnFire · 2023-09-29T10:55:34Z

app.ts

+					alert("An error occurred while trying to search. Please try again.");
+					throw error;


Similar comment about handling the error and then throwing it anyway.

FritzOnFire · 2023-09-29T10:55:56Z

app.ts

+					alert("An error occurred while trying to search. Please try again.");
+					console.error("Failed when clicking on the results: ", error);
+					throw error;


Similar comment about handling the error and then throwing it anyway.

FritzOnFire · 2023-09-29T10:56:16Z

app.ts

+			let endID: number;
+			let pageEnd: number;
+			let pageStart: number;
+			return this.getRecordCount()


Pretty sure the handler function jquery is looking for does not return anything. Please remove the return.

FritzOnFire · 2023-09-29T11:00:04Z

app.ts

+				alert("An error occurred while trying to adjust the records. Please try again.");
+				throw error;


???

So you handle the error... and then you throw it anyway meaning that you have to handle it again later?

Please do not handle the error here, as who ever is calling adjustDisplayedRecords needs to know that we failed to adjustDisplayedRecords and they need to react accordingly.

FritzOnFire · 2023-09-29T11:02:16Z

app.ts

+					alert("An error occurred while trying to resize the window. Please try again.");
+					throw error;


You can't throw here, there is no one left to catch it.

FritzOnFire · 2023-09-29T11:02:52Z

app.ts

+		})
+		.catch(error => {
+			alert("An error occurred while trying to load your page. Please try again.");
+			throw new Error(`An error occurred when loading your page: ${error}`);


You can't throw here, there is no one left to catch it.

MaxTF141 added 12 commits August 14, 2023 09:06

update

3a4e462

Fixed the table filling the entire screen and started on the search. …

8a555a7

…This time by ID.

Added a new function that can load records based on the screen size. …

75dc47a

…Removed my global search function and inserted functionality to only search by id.

Updated functionality to display number of records according to heigh…

476cad9

…t. Updated pagination so it can work with the adjustDisplayedRecords function.

Fixed resizing issue by putting a debounce delay on the window resize.

b11aad3

updated my pagination in relation to the search function

6d8b880

During resizing this version of the project change the start and end …

ecf387c

…of records displayed. It depends on the screen height and the page.

updated. When adjusting height the first record of the page stays the…

441ca1d

… same.

update

c948341

Merge branch 'master' of github.com:MaxTF141/onboard-javascript into …

700d931

…HEAD

updated

090f7cd

Fixed my last value's page and the pagination when going back and for…

9005d45

…th page rows. App doesn't crash when resizing too low.

MaxTF141 requested review from AshtonMar, CelesteNaude, DevinFledermaus, FritzOnFire and anzelIMQS August 24, 2023 13:15

MaxTF141 added 2 commits August 24, 2023 15:17

Fixed my paginapages when resizing and starting id's on smaller numbe…

86fb0dd

…rs. Finished with all the functionalities.

Formatted

3b5990f

DevinFledermaus reviewed Aug 28, 2023

View reviewed changes

app.ts Outdated

return columns;

} catch (error) {

console.error(error);

Copy link

DevinFledermaus Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to console log

DevinFledermaus reviewed Aug 28, 2023

View reviewed changes

DevinFledermaus suggested changes Aug 28, 2023

View reviewed changes

MaxTF141 added 2 commits August 28, 2023 16:55

Almost done with the requested changes

db01e42

Restructured my global variables and changed it to properties.

2764976

MaxTF141 requested a review from DevinFledermaus August 30, 2023 06:18

Added semicolons

df647ea

FritzOnFire requested changes Sep 11, 2023

View reviewed changes

MaxTF141 and others added 10 commits September 11, 2023 16:49

update

3fb7c44

Delete apiManager.js

ad77763

Delete ApiManager.js

8704cb7

Delete apiManager.js.map

cff0b57

Delete ApiManager.js.map

3198ed0

removed js files

530cef4

Merge branch 'master' of github.com:MaxTF141/onboard-javascript

48d8158

Deleted the js files

requested changes still in progress

53ca9e5

set starting value of records to zero and made small requested change…

765552d

…s. Fixed bugs that poped up after changing starting id.

Did the requested changes.

59da1fc

MaxTF141 requested a review from FritzOnFire September 15, 2023 13:39

update

d9882f8

anzelIMQS requested changes Sep 19, 2023

View reviewed changes

Did the requested changes

18ee383

MaxTF141 requested a review from anzelIMQS September 20, 2023 08:41

small update

ff94cd8

anzelIMQS requested changes Sep 20, 2023

View reviewed changes

MaxTF141 added 2 commits September 22, 2023 08:19

Did most of the requested changes

99ca15c

Updated calculations for more precision

12164f0

MaxTF141 requested a review from anzelIMQS September 26, 2023 09:13

Updated comments.

43bef10

FritzOnFire requested changes Sep 27, 2023

View reviewed changes

Did all the requested changes including error handling

9b9bcc6

MaxTF141 requested a review from FritzOnFire September 29, 2023 10:55

FritzOnFire requested changes Sep 29, 2023

View reviewed changes

MaxTF141 added 2 commits September 29, 2023 15:28

Update

e2e0439

Update

1d6c7ba

MaxTF141 requested a review from FritzOnFire October 2, 2023 06:24

		let difference = 0


		// This function will handle retrieving the records from the api

	const columns: Array<string> = await data.json();
	const columns: string[]= JSON.parse(data);

	throw new Error(`Failed to retrieve columns: ${error}`)
	throw new Error(`Failed to retrieve columns: ${error}`);

	}
	else if (this.currentPage === 1) {
	} else if (this.currentPage === 1) {

	throw new Error("Please provide a valid integer.")
	throw new Error("Please provide a valid integer.");

		private fetchJson(mainUrl: string): Promise<any> {
		return fetch(mainUrl)

	showRecords(fromID: number, toID: number): Promise<any> {
	showRecords(fromID: number, toID: number): Promise<void> {

	this.apiManager = new ApiManager("http://localhost:2050/");
	this.apiManager = new ApiManager("http://localhost:2050");

	return this.fetchJson(`${this.mainUrl}records?from=${fromID}&to=${toID}`);
	return this.fetchJson(`${this.mainUrl}/records?from=${fromID}&to=${toID}`);

	return this.fetchJson(`${this.mainUrl}columns`);
	return this.fetchJson(`${this.mainUrl}/columns`);

	if (inputNumberInt < 1000000 && inputNumberInt > 0) {
	if (inputNumberInt < 1000000 && inputNumberInt > 0) {

	return Promise.resolve(undefined);
	return new Promise<void>(() => { });

	return this.apiManager.getRecordCount()
	this.apiManager.getRecordCount()

		@@ -0,0 +1,408 @@
		class DataHandler {
		/** It tracks if the showRecords function is running and would the value would be false if it's not. */

		alert("An error occurred while trying to load the page. Please try again.");
		throw error;

Onboarding Project: Maxwill #48

Are you sure you want to change the base?

Onboarding Project: Maxwill #48

Uh oh!

Conversation

MaxTF141 commented Aug 14, 2023

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!

DevinFledermaus 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!

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!

anzelIMQS 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!

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!

anzelIMQS 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!