Onboarding Project: Meezaan #49

MeezaanD · 2023-08-14T07:10:55Z

No description provided.

…ange dynamically

DevinFledermaus · 2023-08-28T11:14:11Z

app.ts

+let searchResultDisplay = false;
+let isSearchMode = false;
+let searchResultIndexes: number[] = [];
+let specialResizeOccurred = false;


Try sticking to two global variables. And if you set a variable no need to give the type aswell

DevinFledermaus · 2023-08-28T11:14:51Z

app.ts

+  handleSpecialResize();
+};
+// Global variables
+const IMQS: string = "http://localhost:2050";


camelCase please

Please ignore, as for global constant values we are supposed to always use all caps.

DevinFledermaus · 2023-08-28T11:16:59Z

app.ts

+    let recordCount: number = await (await fetch(`${IMQS}/recordCount`)).json();
+    totalRecordCount = recordCount;


Suggested change

let recordCount: number = await (await fetch(`${IMQS}/recordCount`)).json();

totalRecordCount = recordCount;

let recordCount: number = await fetch(`${IMQS}/recordCount`);

totalRecordCount = JSON.parse(recordCount);

DevinFledermaus

Could you also add a space between the functions

DevinFledermaus · 2023-09-01T08:16:18Z

app.ts

+const IMQS = "http://localhost:2050";
+let totalRecordCount: number;
+let totalPages: number;
+let currentValueOfFirstRecord = 1;
+let currentFirstRecordIndex = 1;
+let currentPage: number;
+let recordsPerPage: number;
+let searchedIndex: number | null = null;
+let resizeTimeout: number;


Way too many global variables, try for max two

DevinFledermaus · 2023-09-01T08:18:31Z

app.ts

+async function totalRecords(): Promise<void> {
+	try {
+		let recordCountResponse = await fetch(`${IMQS}/recordCount`);
+		let recordCountText = await recordCountResponse.text();


You don't need to await here

DevinFledermaus · 2023-09-01T08:21:30Z

app.ts

+async function displayColumns() {
+	try {
+		let columnsResponse = await fetch(`${IMQS}/columns`);
+		let columns = await columnsResponse.json();


Don't need to await here

DevinFledermaus · 2023-09-01T08:28:06Z

app.ts

+		let columnsResponse = await fetch(`${IMQS}/columns`);
+		let columns = await columnsResponse.json();
+		let tableHeaderRow = $("#tableHeaderRow");
+		columns.forEach((columnName: string) => {


Could you use a for..of loop instead

DevinFledermaus · 2023-09-01T08:42:14Z

app.ts

+		let response = await fetch(
+			`${IMQS}/records?from=${fromRecord}&to=${fromRecord + maximumRecords - 1
+			}`
+		);


You making this call multiple times, try having it a function of it's own, similar to totalRecords, and that way displayData wont need to be wont need to be an async function

DevinFledermaus · 2023-09-01T09:06:39Z

app.ts

+		throw (error)
+	};
+};
+async function updatePages(isNavigation = false) {


Wont be an async when displayData is changed

DevinFledermaus · 2023-09-01T09:07:09Z

app.ts

+		const recordsText = await response.text();
+		const records = JSON.parse(recordsText);


Suggested change

const recordsText = await response.text();

const records = JSON.parse(recordsText);

const records = JSON.parse(response.text());

DevinFledermaus · 2023-09-01T09:08:55Z

app.ts

+			const searchPageIndex = searchedIndex % recordsPerPage;
+			currentPage = searchPage;
+			currentFirstRecordIndex = (searchPage - 1) * recordsPerPage;
+			await displayData(currentFirstRecordIndex, recordsPerPage);


Suggested change

await displayData(currentFirstRecordIndex, recordsPerPage);

displayData(currentFirstRecordIndex, recordsPerPage);

DevinFledermaus · 2023-09-01T09:09:19Z

app.ts

+		await searchMethod(currentValueOfFirstRecord);
+	}
+	let fromRecord = currentFirstRecordIndex;
+	await displayData(fromRecord, recordsPerPage);


Suggested change

await displayData(fromRecord, recordsPerPage);

displayData(fromRecord, recordsPerPage);

DevinFledermaus · 2023-09-01T09:11:53Z

app.ts

+	} catch (error) {
+		throw (error)
+	};
+};


No ; after the declaration of functions

anzelIMQS

Partial review

anzelIMQS · 2023-09-11T08:37:55Z

app.ts

+			return recordCount;
+		}).catch((error) => {
+			throw error;
+		})


Suggested change

})

});

Missing semi-colon

anzelIMQS · 2023-09-11T08:40:20Z

app.ts

+		.then((dataText) => JSON.parse(dataText))
+		.catch((error) => {
+			throw error;
+		})


Suggested change

})

});

Missing semi-colon

anzelIMQS · 2023-09-11T08:44:15Z

app.ts

+		.then((response) => response.text())
+		.then((dataText) => JSON.parse(dataText))


You could just say

Suggested change

.then((response) => response.text())

.then((dataText) => JSON.parse(dataText))

.then((response) => response.json())

Instead of first to text and then JSON.parse.
This is applicable to your other fetches as well.
If it is not valid json, the catch will throw your error at least.

anzelIMQS · 2023-09-11T08:49:50Z

app.ts

+}
+
+// Fetch column names and creates them as table headings
+function fetchColumns() {


Add a return type. You are returning a promise of something or void?

anzelIMQS · 2023-09-11T08:52:38Z

app.ts

+			updateScreen();
+		}).catch((error) => {
+			throw error;
+		})


Suggested change

})

});

Missing semi-colon

anzelIMQS · 2023-09-11T09:16:05Z

app.ts

+}
+
+// Search for records by a given value and display them on the page.
+async function searchMethod(searchValue: any) {


Suggested change

async function searchMethod(searchValue: any) {

async searchMethod(searchValue: number): Promise<fill in your return type> {

Give return types and parameter type.

anzelIMQS · 2023-09-11T09:17:41Z

app.ts

+	searchValue = Math.min(searchValue, (await totalRecords()) - 1);
+	let targetPage = Math.ceil((searchValue + 1) / state.recordsPerPage);
+	targetPage = Math.min(targetPage, totalPages);
+	const lastRecordIndex = (await totalRecords()) - 1;


You really shouldn't have to call totalRecords multiple times in the same function. Call it once and store the value that the promise returns. Handle it.

anzelIMQS · 2023-09-11T09:19:34Z

app.ts

+			} else {
+				$("#nextPageButton").show();
+			}
+		}).catch((error) => {


Suggested change

}).catch((error) => {

})

.catch((error) => {

Put catches on the next line like the suggestion. The same like you did with the thens

anzelIMQS · 2023-09-11T09:27:49Z

app.ts

+		}
+	})
+
+	$("#searchForm").submit(async function (e) {


Suggested change

$("#searchForm").submit(async function (e) {

$("#searchForm").submit(async (e) => {

anzelIMQS · 2023-09-11T09:29:02Z

app.ts

+		await searchMethod(searchValue);
+		if (state.searchedIndex !== null) {
+			currentPage = Math.ceil((state.searchedIndex + 1) / state.recordsPerPage);
+			state.currentFirstRecordIndex = Math.max(state.searchedIndex - state.recordsPerPage + 1, 0);
+		};
+		$("#loader").hide();
+		$("#tableWrapper").show();


Suggested change

await searchMethod(searchValue);

if (state.searchedIndex !== null) {

currentPage = Math.ceil((state.searchedIndex + 1) / state.recordsPerPage);

state.currentFirstRecordIndex = Math.max(state.searchedIndex - state.recordsPerPage + 1, 0);

};

$("#loader").hide();

$("#tableWrapper").show();

searchMethod(searchValue)

.then(() => {

if (state.searchedIndex !== null) {

currentPage = Math.ceil((state.searchedIndex + 1) / state.recordsPerPage);

state.currentFirstRecordIndex = Math.max(state.searchedIndex - state.recordsPerPage + 1, 0);

}

$("#loader").hide();

$("#tableWrapper").show();

});

Handle your promise rather. It feels cleaner than await. Excuse the indentation in the suggestion.

FritzOnFire · 2023-09-11T12:58:01Z

app.ts

+let totalPages: number;
+let currentPage: number;
+
+const state = {
+	IMQS: "http://localhost:2050",
+	currentValueOfFirstRecord: 1,
+	currentFirstRecordIndex: 1,
+	recordsPerPage: 16,
+	searchedIndex: null as number | null,
+	isButtonDisabled: false
+}


Global variables are bad, please remove them.

I noticed that previous comments on the PR indicated a number of global variables to aim for. Please ignore that, as it was meant to be a worst case scenario in case the creator of the PR could not figure out how to do it.

I would recommend moving them to a class along with the methods that are depended on them.

FritzOnFire · 2023-09-11T13:00:02Z

app.ts

+	isButtonDisabled: false
+}
+
+// Fetch the total number of records from the server.


Please change this comment to match the javadoc style. This is unfortunately not currently in our style guide.

FritzOnFire · 2023-09-11T13:01:28Z

app.ts

+// Fetch the total number of records from the server.
+function totalRecords(): Promise<number> {
+	return fetch(`${state.IMQS}/recordCount`)
+		.then((recordCountResponse) => recordCountResponse.json())


Please remove the () around recordCountResponse or specify a type.

Also, you need to check if the response was good, as .catch is not going to catch that for you.

FritzOnFire · 2023-09-11T13:03:50Z

app.ts

+		});
+}
+
+// Fetch column names and creates them as table headings


Similar javadoc style comment.

FritzOnFire · 2023-09-11T13:04:52Z

app.ts

+// Fetch column names and creates them as table headings
+function fetchColumns(): Promise<void> {
+	return fetch(`${state.IMQS}/columns`)
+		.then((columnsResponse) => columnsResponse.json())


Either remove the () around columnsResponse or specify a type.

Also, you need to check the response was good, as .catch is not going to catch that for you.

FritzOnFire · 2023-09-11T15:21:03Z

index.html

+    <button id="prevPageButton">Previous Page</button>
+    <form id="searchForm">
+      <div class="form-group">
+        <input type="number" id="searchInput" placeholder="Search ID" min="1" required />


So I can't search for the first record? Bit of an odd choice.

FritzOnFire · 2023-09-11T15:22:24Z

style.css

Please use tabs as indentation.

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

style.css

+/* Colors */
+:root {
+  --primary-color: white;
+  --secondary-color: #292929;
+  --tertiary-color: black;
+  --shadow-color: #e3e3e3;
+  --selected-color: #0d6efd;
+}
+


Definitely want to set your variables first in this file. I would recommend moving it up to be the first thing in this file.

FritzOnFire · 2023-09-11T15:28:11Z

style.css

+	background: var(--selected-color);
+	box-shadow: none !important;
+	border: 1px solid var(--secondary-color);
+  }


Formatting got a bit weird here

FritzOnFire · 2023-09-11T15:28:39Z

style.css

Please ensure an empty line between classes.

…style comments

FritzOnFire · 2023-09-18T14:10:22Z

app.ts

+	currentValueOfFirstRecord: number = 0;
+	/** Index of the first record currently displayed on the page */
+	currentFirstRecordIndex: number = 0;
+	/** Current page number (changes dynamically)*/


Suggested change

/** Current page number (changes dynamically)*/

/** Current page number (changes dynamically) */

FritzOnFire · 2023-09-18T14:10:29Z

app.ts

+	recordsPerPage: number = 16;
+	/** Index of the record being searched for (null if not searching) */
+	searchedIndex: number | null = null;
+	/** Checks if the button is enabled/disabled*/


Suggested change

/** Checks if the button is enabled/disabled*/

/** Checks if the button is enabled/disabled */

FritzOnFire · 2023-09-18T14:14:48Z

app.ts

+		const loadApp = new Promise<void>((resolve, reject) => {
+			window.onload = () => {
+				try {
+					$("#loader").hide();
+					$(window).on("resize", this.debounce(() => {
+						this.updateScreen();
+					}, 250));
+					this.fetchColumns();
+					this.updateScreen();
+					this.eventHandlers();
+					resolve();
+				} catch (error) {
+					reject(error);
+				}
+			};
+		});
+		loadApp
+			.catch(() => {
+				throw new Error('Error during window.onload:');
+			});


Please do this somewhere else... a constructor should not be doing anything that can result in an error being thrown. And it should also definitely not be doing any asynchronous calls.

FritzOnFire · 2023-09-18T14:23:45Z

app.ts

+				if (!recordCountResponse.ok) {
+					throw new Error('Error trying to get recordCount');
+				}
+				return recordCountResponse.json();


Pretty sure /recordCount does not return a json... Pretty sure you want to return text() and then parse the text in the next .then()

FritzOnFire · 2023-09-18T14:25:18Z

app.ts

+			.then(columnsResponse => {
+				if (!columnsResponse.ok) {
+					throw new Error('Error trying to fetch the columns');
+					return [];


Please remove this return, as you should not have a return statement after a throw

FritzOnFire · 2023-09-19T14:01:24Z

app.ts

+			if (this.currentPage >= this.totalPages) {
+				const errorMessage = "Already on the last page";
+				window.alert(errorMessage);
+				throw new Error(errorMessage);


You are the last line of defense, you can't throw an error here. But it looks like you are all ready handling the error with window.alert, so I would just replace this line with a return.

FritzOnFire · 2023-09-19T14:04:14Z

app.ts

+			$("#prevPageButton").hide();
+			$("#tableWrapper").hide();
+			$("#loader").show();
+			let totalRecCount: number | void;


This value is not set anywhere... I think you meant to place it in the .then that handles the promise coming from totalRecords.

FritzOnFire · 2023-09-19T14:05:20Z

app.ts

+					return this.displayData(fromRecord, this.recordsPerPage);
+				})
+				.catch((error) => {
+					throw error;


You are the last line of defense, you need to handle the error here.

FritzOnFire · 2023-09-19T14:08:31Z

app.ts

+}
+
+/** Initialize the app when the page loads */
+new InitializeApp();


This feels out of place without the window.onload = () => { ... }; mentioned in the readme of this project. Not saying your code has to look like that, but I would struggle to have something to comment about if you still used the example that we provided in the readme.

FritzOnFire · 2023-09-19T14:09:01Z

index.html

This comment is still relevant.

… function

MeezaanD added 13 commits August 14, 2023 08:55

first push

24da354

update to search

f3ec479

update on how I display records on the load up page

a1270ce

fixed the filtering issues and displaying on the large screen

77728c9

fixed the issue where I can do the next page on the last page

fc0e2d9

updates on resizing not completely functional yet

7c0db77

upadtes on resizing according to the searchResults

0614c90

add page number out total pages display on the dom

93d1277

changed the page to update according to the first record to always ch…

320c076

…ange dynamically

added a background-color to search results

f6ae00b

first record on different page update

fba4223

updates to last page

e1ea02b

new changes with the resize

504dabe

DevinFledermaus reviewed Aug 28, 2023

View reviewed changes

MeezaanD added 3 commits August 28, 2023 16:16

upadtes

f7d63df

working search after resizing issue

4e33773

updates 30 Aug

7eb6cf8

MeezaanD requested review from AshtonMar, CelesteNaude, DevinFledermaus, FritzOnFire and anzelIMQS August 30, 2023 09:53

MeezaanD added 3 commits August 30, 2023 11:55

updates

0b79c4a

updates Aug 30

55e4984

worked on navigations with buttons regarding to first and last page

df6f87d

DevinFledermaus suggested changes Sep 1, 2023

View reviewed changes

MeezaanD added 2 commits September 1, 2023 16:35

first pull request changes

e188ba6

reduced global variables and other pull request changes

56968ae

first round of pull request changes made

9033e9c

MeezaanD requested a review from DevinFledermaus September 6, 2023 14:35

MeezaanD added 2 commits September 7, 2023 13:41

updates

1297016

updates 8 Sep

5bea166

anzelIMQS requested changes Sep 11, 2023

View reviewed changes

updates 11 Sep

9c91a08

FritzOnFire requested changes Sep 11, 2023

View reviewed changes

MeezaanD added 4 commits September 12, 2023 15:25

updates 12 Sep

6d189b1

switching to class

ca099e1

updated async functions to normal with adjustments and added javadoc …

d41e2f4

…style comments

chnages 14 Sep

b0e10fb

MeezaanD requested review from FritzOnFire and anzelIMQS September 14, 2023 10:18

MeezaanD added 4 commits September 14, 2023 14:48

small updates

4b1bb4b

removed unnecessary spaces

3211f9e

a few type adjustments

5fb8789

updates 15 Sep

581729c

FritzOnFire requested changes Sep 19, 2023

View reviewed changes

MeezaanD added 2 commits September 20, 2023 12:51

updates Wed 20 morning

8663db3

request for pr review

b578caa

MeezaanD requested a review from FritzOnFire September 21, 2023 07:02

MeezaanD added 8 commits September 21, 2023 12:12

removed the unnecessary return of the displayData on the updateScreen…

510571e

… function

update

c9d1b4c

css updates

3649ab6

removed unnecessary returns

042d9ac

small changes

a00ccbc

removed unnecessary brackets

966ea03

update

f0fb7a2

update Oct 9

65fb57b

MeezaanD self-assigned this Jan 24, 2024

		let recordCount: number = await (await fetch(`${IMQS}/recordCount`)).json();
		totalRecordCount = recordCount;

		const recordsText = await response.text();
		const records = JSON.parse(recordsText);

	const recordsText = await response.text();
	const records = JSON.parse(recordsText);
	const records = JSON.parse(response.text());

	await displayData(currentFirstRecordIndex, recordsPerPage);
	displayData(currentFirstRecordIndex, recordsPerPage);

	await displayData(fromRecord, recordsPerPage);
	displayData(fromRecord, recordsPerPage);

		.then((response) => response.text())
		.then((dataText) => JSON.parse(dataText))

	.then((response) => response.text())
	.then((dataText) => JSON.parse(dataText))
	.then((response) => response.json())

	async function searchMethod(searchValue: any) {
	async searchMethod(searchValue: number): Promise<fill in your return type> {

	$("#searchForm").submit(async function (e) {
	$("#searchForm").submit(async (e) => {

	/** Current page number (changes dynamically)*/
	/** Current page number (changes dynamically) */

	/** Checks if the button is enabled/disabled*/
	/** Checks if the button is enabled/disabled */

Onboarding Project: Meezaan #49

Are you sure you want to change the base?

Onboarding Project: Meezaan #49

Uh oh!

Conversation

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

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!

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