Onboarding Project by Ntobe99 · Pull Request #46 · IMQS/onboard-javascript

Ntobe99 · 2023-08-11T13:36:21Z

Please Review

DevinFledermaus · 2023-08-28T08:38:52Z

index.html

+			  </div>
+
+			</div>	
+	<script src="/app.js"></script>


The script tag can be moved into head tag

DevinFledermaus · 2023-08-28T10:10:22Z

app.ts

+			console.error('Failed to fetch record count', error);
+			throw error;


Suggested change

console.error('Failed to fetch record count', error);

throw error;

throw new Error('Failed to fetch record count', error)

DevinFledermaus · 2023-08-28T10:10:42Z

app.ts

+			this.data = new Array<GridData>(this.columnNames.length);
+		} catch (error) {
+			console.error('Failed to fetch columns', error);
+			throw error;


same as above

DevinFledermaus · 2023-08-28T10:42:35Z

app.ts

+				this.displayRecords();
+				this.updatePageInfo();
+			}
+			else {


move the else to the same line as the }

FritzOnFire

One small comment, but from my side I think you are done.

@anzelIMQS @CelesteNaude Can I ask both of you to give this a once over, just in case there is anything major that I looked over. But I feel that we have covered enough to call this done.

FritzOnFire · 2023-09-14T14:40:57Z

ApiData.ts

+	private setupControls(): void {
+		$('#prevBtn').on('click', () => this.handlePageChange(-1));
+		$('#nextBtn').on('click', () => this.handlePageChange(1));
+		$(window).on('resize', debounce(this.handleResize.bind(this), 100));


We prefer to use arrow functions instead of using .bind(this).

Suggested change

$(window).on('resize', debounce(this.handleResize.bind(this), 100));

$(window).on('resize', debounce(() => { this.handleResize(); }, 100));

anzelIMQS · 2023-09-05T11:43:18Z

ApiData.ts

+			.then(() => this.recordCount())
+			.then(() => this.fetchColumns())
+			.then(() => this.fetchRecords())
+			.then(() => this.setupControls())


Suggested change

.then(() => this.setupControls())

.then(() => this.setupControls());

Missing semi-colon

anzelIMQS · 2023-09-15T09:02:38Z

ApiData.ts

+			.then((response: number | string) => {
+				const res = JSON.parse(<string>(response));


Suggested change

.then((response: number | string) => {

const res = JSON.parse(<string>(response));

.then((response: string) => {

const res = JSON.parse(response);

Isn't just always a string? I would think that the /columns response brings back a number.

anzelIMQS · 2023-09-15T09:04:11Z

ApiData.ts

+			.then((response: number | string) => {
+				const totalItems = <number>response;
+				this.totalItems = totalItems;


Suggested change

.then((response: number | string) => {

const totalItems = <number>response;

this.totalItems = totalItems;

.then((response: number) => {

this.totalItems = response;

I also think this can only be a number once it is successful.

anzelIMQS · 2023-09-15T09:07:24Z

ApiData.ts

+			.then((response: number | string) => {
+				const res = JSON.parse(<string>(response));


Suggested change

.then((response: number | string) => {

const res = JSON.parse(<string>(response));

.then((response: string) => {

const res = JSON.parse(response);

It will never return a number and if one day it does, we know something fishy is going on and at least you would log that?

I would suggest return response.json() instead of JSON.parse(response) and handling your promises' error in your catch. Just a suggestion though. You don't have to as long as you log the error in your catch..

anzelIMQS · 2023-09-15T09:15:32Z

ApiData.ts

+			.catch(() => {
+				throw ('Failed to fetch records');


Suggested change

.catch(() => {

throw ('Failed to fetch records');

.catch(error => {

throw ('Failed to fetch records: ', error);

I mean, wouldn't you like to know the error? We know if failed but why?

anzelIMQS · 2023-09-15T09:33:35Z

ApiData.ts

+				this.updatePageInfo();
+			})
+			.catch(error => {
+				console.error('Failed to fetch records:', error);


Technically speaking, what do you think your console.error will look like since you have nested promises?
Think about it 🤔

anzelIMQS · 2023-09-15T09:35:45Z

ApiData.ts

+				this.displayRecords();
+				this.updatePageInfo();


So this.displayRecords() calls this.updatePageInfo() and then we call it again?

Sounds like double work to me 🥵

anzelIMQS · 2023-09-15T09:36:06Z

ApiData.ts

+					this.displayRecords();
+					this.updatePageInfo();


Same vibe as previously mentioned

anzelIMQS · 2023-09-15T09:40:10Z

ApiData.ts

+	}
+
+	private displayRecords(): void {
+		const gridTemplate = new GridTemplate(this.columnNames, this.data);


So we are creating the GridTemplate class everytime we displayRecords()?

I would have loved that we could just set the dataRecords every time we update the display because columnNames never changes throughout this life. It just sounds like a lot of objects being created and created and created...

anzelIMQS · 2023-09-15T09:43:00Z

ApiData.ts

+
+		this.fetchAndDisplayRecords()
+			.then(() => {
+				this.updatePageInfo();


Please look at the number of times you call this.updatePageInfo() and see if it really is necessary? I would think fetchAndDisplayRecords() already called it for you and this.displayRecords() as well and now this.handlePageChange() as well.

CelesteNaude

Couldn't find any further major changes.

anzelIMQS

Something small but I think after this you should be home free.

anzelIMQS · 2023-09-20T16:20:57Z

ApiData.ts

+				const totalItems =response;
+				this.totalItems = totalItems;


Suggested change

const totalItems =response;

this.totalItems = totalItems;

this.totalItems = response;

anzelIMQS · 2023-09-20T16:22:58Z

ApiData.ts

+	/** Use the fetchData() func to make an HTTP request to the API endpoint and process the data */
+	fetchColumns(): Promise<void> {
+		return this.fetchStrData('http://localhost:2050/columns')
+			.then((response:string) => {


Suggested change

.then((response:string) => {

.then((response: string) => {

anzelIMQS · 2023-09-20T16:23:07Z

ApiData.ts

+		$('#grid').hide();
+
+		return this.fetchStrData(`http://localhost:2050/records?from=${from}&to=${to}`)
+			.then((response:string) => {


Suggested change

.then((response:string) => {

.then((response: string) => {

anzelIMQS · 2023-09-20T16:57:52Z

ApiData.ts

+		// Maximum allowed Value
+		const maxRange = this.maxRange;
+
+		if (searchValue >= 0 && searchValue <= maxRange) {
+			let from = searchValue;
+			const pageSize = this.pageSize;
+
+			if (from + pageSize > maxRange) {
+				from = Math.max(0, maxRange - pageSize);
+			}


So how much different is this logic compared to your fetchAndDisplayRecords() logic?

The way I see it you can set this.firstVal = searchValue and let nature take its course by just calling fetchAndDisplayRecords(), or am I missing something?

Similar to what you did with handleResize

anzelIMQS · 2023-09-20T17:02:36Z

ApiData.ts

+					this.currentPage = Math.ceil(from / this.pageSize) + 1;
+					// Set firstVal to searched value
+					this.firstVal = from;
+					// Calculate lastVal based on pageSize
+					this.lastVal = from + this.pageSize - 1;


With the above comment being said. This could then be executed before the time and life would be good, right?

anzelIMQS

LGTM, just the one comment but no other major issues to comment on.

anzelIMQS · 2023-09-29T12:02:20Z

ApiData.ts

+
+	/** Fetches records using fetchAndProcessRecords(), processes them, displays them, and updates page information. */
+	fetchAndDisplayRecords(): Promise<void> {
+		this.maxRange = this.totalItems - 1;


Please move this line to your recordCount() as this value does not change but you are setting it over and over again with the same value when you do a fetchAndDisplayRecords.

FritzOnFire

There are some comments, but I don't think they are that major.

I would once again say that, you are done.

FritzOnFire · 2023-10-02T07:52:54Z

ApiData.ts

+				alert('Failed to fetch record count.');
+				throw error;


???

You handle the error... but then you throw it anyway? Rather just throw an error here, and handle the error as late as possible.

FritzOnFire · 2023-10-02T07:55:12Z

ApiData.ts

+		return this.fetchStrData(`http://localhost:2050/records?from=${from}&to=${to}`)
+			.then((response: string) => {
+				const res = JSON.parse(response);
+				const processedData = res.map((record: string) => {


I don't think the type you have for record is correct

FritzOnFire · 2023-10-02T08:01:34Z

ApiData.ts

+			$('#fromInput').val('');
+			return this.fetchAndDisplayRecords();
+		} else {
+			alert(`Error while searching, please enter values in the range (0-${this.maxRange})`);


You don't need to make an changes, just a comment.

I find it interesting that you would handle the error here instead of throwing it and letting the calling function deal with it, but it kinda makes sens, I can't imagine doing anything after the function that needs it to succeed.

FritzOnFire · 2023-10-02T08:08:22Z

ApiData.ts

+	private async fetchNumData(url: string): Promise<number> {
+		const response = await $.ajax({
+			url,
+			method: 'GET',
+		});
+		return response;
+	}


I really want to comment on how you don't know if response is actually a number... but I am finding it hard to phrase it... but I guess for this project it is fine.

Ntobe99 added 14 commits August 1, 2023 15:23

structure and some functionalality

9de206d

grid and controls added

feddf76

search using id working

20ab5f0

spinner added

161dba8

some changes

4db8da4

changes to code format

4379d03

scrollbar removed and empty array debug

1574a99

dyanmic grid and some search functionality

d7cc5a7

grid has range (0-999999) and some search functionality

68d6fdc

stationary firstVal and improved search functionality

5590608

firstpage functionality improved

92c2304

firstpage functionality improved

0449b6f

mmm

18305ee

mmm

07b2e55

Ntobe99 requested review from AshtonMar, CelesteNaude, DevinFledermaus, FritzOnFire and anzelIMQS August 25, 2023 11:26

Ntobe99 added 4 commits August 25, 2023 14:05

final

46fa399

final

120a175

formatting added

0250464

formatting added

e79a514

DevinFledermaus reviewed Aug 28, 2023

View reviewed changes

index.html Outdated

</div>

</div>

<script src="/app.js"></script>

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.

The script tag can be moved into head tag

Ntobe99 and others added 3 commits August 28, 2023 11:20

Update app.ts

8df21a6

script tag moved to head tag

b2c0a70

script tag moved to head tag

b16c29c

DevinFledermaus reviewed Aug 28, 2023

View reviewed changes

app.ts Outdated

this.displayRecords();

this.updatePageInfo();

}

else {

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.

move the else to the same line as the }

Ntobe99 requested a review from FritzOnFire September 14, 2023 11:01

FritzOnFire approved these changes Sep 15, 2023

View reviewed changes

requested changes done

7c7ea1a

anzelIMQS requested changes Sep 15, 2023

View reviewed changes

ThokozaniNqwili requested a review from FritzOnFire September 15, 2023 10:51

Ntobe99 added 3 commits September 18, 2023 09:05

some changes done

1df95c0

some changes done

9518331

some changes done

8093c8a

Ntobe99 requested a review from anzelIMQS September 19, 2023 10:08

CelesteNaude approved these changes Sep 19, 2023

View reviewed changes

anzelIMQS requested changes Sep 20, 2023

View reviewed changes

Ntobe99 added 2 commits September 21, 2023 10:17

changes to search and a few

dc20c08

changes to search and a few changes

6fd8954

Ntobe99 requested a review from anzelIMQS September 21, 2023 10:19

Ntobe99 added 9 commits September 21, 2023 12:29

removed unnecessary maxrange var

5ffd990

some changes to logic

8e9d50a

some changes to logic

f2b7bf3

accounted for zero based indexing

138f01c

27/09

e94ac2e

logic updated

cd397ac

logic updated

e2d638d

style updated for width resize

27e027a

resize logic updated

126a1f0

anzelIMQS approved these changes Sep 29, 2023

View reviewed changes

Ntobe99 added 2 commits October 2, 2023 08:14

changes requested done

03274eb

changes requested done

194d261

FritzOnFire requested changes Oct 2, 2023

View reviewed changes

Ntobe99 added 2 commits October 2, 2023 16:01

changes requested done

4d61536

changes requested done for real this time

350c90b

Ntobe99 requested a review from FritzOnFire October 4, 2023 06:03

		console.error('Failed to fetch record count', error);
		throw error;

	console.error('Failed to fetch record count', error);
	throw error;
	throw new Error('Failed to fetch record count', error)

	$(window).on('resize', debounce(this.handleResize.bind(this), 100));
	$(window).on('resize', debounce(() => { this.handleResize(); }, 100));

	.then(() => this.setupControls())
	.then(() => this.setupControls());

		.then((response: number \| string) => {
		const res = JSON.parse(<string>(response));

	const totalItems =response;
	this.totalItems = totalItems;
	this.totalItems = response;

Conversation

Ntobe99 commented Aug 11, 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!

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

CelesteNaude left a comment

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!

anzelIMQS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants