Onboarding Project: Maxwill by MaxTF141 · Pull Request #48 · IMQS/onboard-javascript

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

+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

+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

+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

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


no need to console log

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

+	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

+		}
+		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

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

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

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

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

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


Please leave an empty line between your class variables and constructor

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

+		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

+		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

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


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

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


Please add a space before the {

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

+    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

+.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

+  @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

+				}
+			})
+			.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

+				}
+				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

+							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

+			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

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


Please remove stray

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

+	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

+class ApiManager {
+	private mainUrl: string;


Space inbetween.

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

+					}
+					$("#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

+		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

+	}
+
+	/** 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

+
+	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

+
+	/** 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

+
+	/** 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

+
+	/** 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

+		$(".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

+							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

+	/** 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

+		// 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

+					$(".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

@@ -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

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


Missing ;

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

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


Missing ;

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

+	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

+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

+					}
+					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

+					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

+			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.

FritzOnFire

Please only handle errors as late as possible

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

+		.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

+	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

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


Suggested change

.then (recordCount => {

.then(recordCount => {

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

+			}
+		})
+		.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

+					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

+		// 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

+					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

+					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

+			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

+				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

+					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

+		})
+		.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

Comment thread app.ts Outdated

return columns;

} catch (error) {

console.error(error);

DevinFledermaus Aug 28, 2023

Copy link
Copy Markdown

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;

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment