Skip to content
Open
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
ad2404b
update
ThokozaniNqwili Aug 11, 2023
575b160
update
ThokozaniNqwili Aug 11, 2023
038f93e
update
ThokozaniNqwili Aug 15, 2023
30785ac
update
ThokozaniNqwili Aug 18, 2023
96682be
update
ThokozaniNqwili Aug 23, 2023
e287191
update
ThokozaniNqwili Aug 25, 2023
b4e7f57
format change
ThokozaniNqwili Aug 28, 2023
1f6c3a3
review changes
ThokozaniNqwili Aug 28, 2023
eef44e5
update requested changes
ThokozaniNqwili Aug 29, 2023
097c7c1
update requested changes, index and style
ThokozaniNqwili Aug 29, 2023
3e38dff
update search
ThokozaniNqwili Aug 29, 2023
3c23cb2
first push 30/08
ThokozaniNqwili Aug 30, 2023
d8792ca
onclick
ThokozaniNqwili Aug 30, 2023
55837eb
last push 30/08
ThokozaniNqwili Aug 30, 2023
10b0d40
09/05
ThokozaniNqwili Sep 5, 2023
11ea6c4
update
ThokozaniNqwili Sep 5, 2023
2b53415
update
ThokozaniNqwili Sep 5, 2023
c15fe4c
09/06
ThokozaniNqwili Sep 6, 2023
90d744b
09/08
ThokozaniNqwili Sep 8, 2023
2361a84
11/09
ThokozaniNqwili Sep 11, 2023
eeb1c81
12/09
ThokozaniNqwili Sep 12, 2023
9580c60
15/09
ThokozaniNqwili Sep 15, 2023
cc6a0dd
15/09
ThokozaniNqwili Sep 15, 2023
f18872b
18/09
ThokozaniNqwili Sep 18, 2023
24aba76
18/09
ThokozaniNqwili Sep 18, 2023
2e7e23d
26/09
ThokozaniNqwili Sep 26, 2023
0d8a9d4
update
ThokozaniNqwili Sep 26, 2023
2313df4
update
ThokozaniNqwili Sep 27, 2023
817249c
09/28
ThokozaniNqwili Sep 28, 2023
cb7c3ea
10/02
ThokozaniNqwili Oct 2, 2023
f8204e9
updates
ThokozaniNqwili Oct 2, 2023
249611b
03/10
ThokozaniNqwili Oct 3, 2023
7a4b0e2
04/10
ThokozaniNqwili Oct 5, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/node_modules
app.js
app.js.map
dataManager.js
dataManager.js.map
214 changes: 214 additions & 0 deletions app.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
import { ajax, css } from "jquery";

class RecordManager {
firstNumber: number;
lastNumber: number;
recordCount: number;

constructor() {
this.firstNumber = 0;
this.lastNumber = 0;
this.recordCount = 0;
this.initialize();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not recommend calling functions that can throw errors in a constructor... You could technically still handle the errors, but the rest of your code would think everything went fine, when it very much did not.

Please call this initialize after creating a new instance of RecordManager

}

initialize() {
this.createTableHeader();
dataManager.fetchRecordCount()
.then(count => {
this.recordCount = count - 1;
this.updateAndDisplayRecords();
this.recordEventHandlers();
this.handleResize();
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing ;

.catch(err => {
alert('Error fetching and displaying the table records, reload the page');
throw err;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

???

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 initialize needs to know that we failed to initialize and they need to react accordingly.

});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So... two things.

  1. Returning a promise makes catching errors much much easier for the calling function, so i would recommend always doing it. BUT

  2. Here you are kicking off two async function calls that do not depend on each other, which is beautiful :) This is a great way to speed up an app, but the calling function still needs to be able to handle the error of the function that you are not waiting for. And so now I will be breaking my own rule about not commenting exact code on an onboarding project. This is how you can wait on multiple async functions at the same time:

Suggested change
this.createTableHeader();
dataManager.fetchRecordCount()
.then(count => {
this.recordCount = count - 1;
this.updateAndDisplayRecords();
this.recordEventHandlers();
this.handleResize();
})
.catch(err => {
alert('Error fetching and displaying the table records, reload the page');
throw err;
});
let p: Promise<void>[] = [];
p.push(this.createTableHeader());
let frcp = dataManager.fetchRecordCount()
.then(count => {
this.recordCount = count - 1;
this.updateAndDisplayRecords();
this.recordEventHandlers();
this.handleResize();
})
.catch(err => {
alert('Error fetching and displaying the table records, reload the page');
throw err;
});
p.push(frcp);
return Promise.all(p);

}

/** Initializes the table head */
createTableHeader() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing return type.

return dataManager.fetchColumns()
.then(columns => {
for (const col of columns) {
$(".head").append(`<th>${col}</th>`);
}
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing ;

.catch(err => {
alert('Error creating table heading');
throw err;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar comment as before about handling and then throwing again.

});
}

/** calculates the number of rows that can fit the screen */
getNumberOfCalculatingRows(): number {
const screenHeight = window.innerHeight;
const availableHeight = screenHeight - 110;
const rowHeight = 35;
if (availableHeight <= 0) {
return 0;
} else {
let maxRows = Math.floor(availableHeight / rowHeight);
return maxRows;
}
}

/** fetching records that fit the screen */
updateAndDisplayRecords(): Promise<void> {
$('#loader').show();
this.calculateFirstAndLastNumbers();
this.updateArrowVisibility();
return this.fetchAndDisplayRecords()
.then(() => {
$('#loader').hide();
})
.catch(err => {
alert('Error to fetch and display records, reload the page');
throw err;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar comment as before about handling and then throwing again.

});
}

Comment thread
FritzOnFire marked this conversation as resolved.
/** Calculates the firstNumber and lastNumber */
calculateFirstAndLastNumbers() {
let rowsPerPage = this.getNumberOfCalculatingRows();
if (this.firstNumber < 0) {
this.firstNumber = 0;
}
this.lastNumber = this.firstNumber + (rowsPerPage - 1);
if (this.lastNumber >= this.recordCount) {
this.firstNumber = this.recordCount - (rowsPerPage - 1);
this.lastNumber = this.recordCount;
}
}

updateArrowVisibility() {
if (this.firstNumber === 0) {
$('.arrow-left').hide();
} else {
$('.arrow-left').show();
}
if (this.lastNumber >= this.recordCount) {
$('.arrow-right').hide();
} else {
$('.arrow-right').show();
}
}

fetchAndDisplayRecords(): Promise<void> {
return dataManager.fetchRecords(this.firstNumber, this.lastNumber)
.then(records => {
const inputValue = <string>($('#searchInput').val());
$("#tableBody").empty();
for (const record of records) {
// creates row for each record
$("tbody").append(`<tr class="row"></tr>`);
const lastRow = $(".row:last");
for (const value of record) {
// assign each record to their column in a specified row
lastRow.append(`<td>${value}</td>`);
if (value === inputValue) {
// highlights the searched row
lastRow.css('background-color', '#DDC0B4');
}
}
$("tbody").append(lastRow);
}
$('#page').empty().append(`Showing record: ${this.firstNumber} - ${this.lastNumber}`);
$('#loader').hide();
})
.catch(err => {
alert('Error while displaying records, reload the page');
throw err;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar comment as before about handling and then throwing again.

});
}

Comment thread
FritzOnFire marked this conversation as resolved.
/** recalculates the record range that includes inputValue fromm user */
searchRecordsAndResize() {
let inputValue = <number>($('#searchInput').val());
if(inputValue >= 0 && inputValue <= this.recordCount){
let calculatedRows = this.getNumberOfCalculatingRows();
// divides the calculated max rows in half
const halfRange = Math.floor(calculatedRows / 2);
this.firstNumber = Math.max(0, inputValue - halfRange);
this.lastNumber = Math.min(this.recordCount, this.firstNumber + (calculatedRows - 1));
}else{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Your formatting seems to have broken on these few lines.

alert('Input value must be between 0 and 999999.');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please don't hard code the max value.

}
}

/** Navigates to the next set of records */
navigateToNextPage() {
$('#searchInput').val('');
if (0 <= this.lastNumber && this.lastNumber <= this.recordCount) {
// calculates the first number of the page
this.firstNumber = this.lastNumber + 1;
const calculatedRows = this.getNumberOfCalculatingRows();
// calculates the last number of the page
this.lastNumber = this.firstNumber + (calculatedRows - 1);
}
this.updateAndDisplayRecords();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would recommend returning this to allow the function calling navigateToNextPage to catch and handle the error updateAndDisplayRecords is throwing.

}

navigateToPreviousPage() {
$('#searchInput').val('');
if (0 <= this.firstNumber && this.firstNumber <= this.recordCount) {
const calculatedRows = this.getNumberOfCalculatingRows();
this.lastNumber = this.firstNumber - 1;
this.firstNumber = this.lastNumber - (calculatedRows - 1);
}
this.updateAndDisplayRecords();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would recommend returning this to allow the function calling navigateToPreviousPage to catch and handle the error updateAndDisplayRecords is throwing.

}

/** calls to re-display records when screen is adjusted */
handleResize() {
let resizeTimeout: number;
let inputValue = <string>($('#searchInput').val());
$(window).on('resize', () => {
clearTimeout(resizeTimeout);
resizeTimeout = setTimeout(() => {
$('#loader').show();
if (inputValue !== '') {
this.searchRecordsAndResize();
}
this.updateAndDisplayRecords()
.then(() => {
$('#loader').hide();
})
.catch(err => {
alert('Error occurred while resizing, reload the page');
$('#loader').hide();
throw err;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should not be throwing here. You have already handled the error, and there is no one else who can handle errors above you.

});
}, 250);
});
}

recordEventHandlers() {
$('#btnSearch').on('click', (event) => {
event.preventDefault();
this.searchRecordsAndResize();
this.updateAndDisplayRecords();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You need to handle the error being thrown by updateAndDisplayRecords

});

$('.arrow-right').on('click', () => {
this.navigateToNextPage();
});

$('.arrow-left').on('click', () => {
this.navigateToPreviousPage();
});

$('#searchInput').on('input', () => {
const inputValue = <string>$('#searchInput').val();
if (inputValue !== undefined) {
let sanitizedValue = inputValue.replace(/[^0-9]/g, '');
$('#searchInput').val(sanitizedValue);
}
});
}
}

window.onload = () => {
new RecordManager();
}
51 changes: 51 additions & 0 deletions dataManager.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a very creative way to hide the fact that you are using a global variable called backend. While there is nothing wrong with using static, and even making a class like this, as there truly is a time and a place for everything, its just unfortunately not in this on boarding project. Please remove all the static labels in this class.

This is basically as bad as turning off asynchronous requests in javascript for this project (I have only seen it once, and I am unable to find out how to do it, but I know it exists).

Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
class dataManager {
static backend: string = "http://localhost:2050";

/** fetches the number of records from backend */
static fetchRecordCount(): Promise<number> {
return fetch(`${this.backend}/recordCount`)
.then(res => {
if (!res.ok) {
throw 'Failed to fetch record count';
}
return res.json();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return res.json();
return res.text();

})
.then(totalRecords => {
if(typeof totalRecords !== 'number'){

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice touch :) Now I kinda feel bad for asking you to not use json.

throw new Error('Invalid response format');
}
return totalRecords;
})
.catch(err => {
throw 'Error fetching the record count: ' + err;
});
}

/** fetches columns from backend */
static fetchColumns(): Promise<string[]> {
return fetch(`${this.backend}/columns`)
.then(res => {
if (!res.ok) {
throw 'Failed to fetch columns';
}
return res.json();
})
.catch(err => {
throw 'Error fetching columns' + err;
});
}

/** fetches records from backend */
static fetchRecords(from: number, to: number): Promise<string[]> {
return fetch(`${this.backend}/records?from=${from}&to=${to}`)
.then(res => {
if (!res.ok) {
throw "Sorry, there's a problem with the network";
}
return res.json();
})
.catch(err => {
throw 'Error fetching records from server ' + err;
});
}
}
36 changes: 34 additions & 2 deletions index.html
Original file line number Diff line number Diff line change
@@ -1,13 +1,45 @@
<!DOCTYPE html>
<html>

<head>
<title>JS Onboard Project</title>
<script type="text/javascript" charset="utf-8" src="third_party/jquery-2.0.3.min.js"></script>
<link rel="stylesheet" href="style.css">
<script src="dataManager.js" ></script>
<script src="app.js" ></script>
</head>

<body>
<p>Hello</p>
<div id="loader">
<div class="loader-spinner"></div>
</div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Your formatting is a bit off here.

<h1 class="heading">Javascript Project</h1>
<div id="modal" class="modal">
<div class="modal-content">
<span id="closeModalBtn" class="close">&times;</span>
<p class="content"></p>
</div>
</div>
<div class="search-container">
<form id="searchForm">
<input type="number" id="searchInput" placeholder="Search.." name="search" autocomplete="off">
<button id="btnSearch" class="btnSearch" type="submit" >Submit</button>
</form>
</div>
<div class="showRecords">
<button class="arrow-right"></button>
<div id="page"></div>
<button class="arrow-left"></button>
</div>
<table id="recordsTable">
<thead>
<tr class="head">
</tr>
</thead>
<tbody id="tableBody">
</tbody>
</table>

</body>

</html>

27 changes: 27 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"name": "onboard-javascript",
"version": "1.0.0",
"description": "This is a JavaScript project for all new developers to complete before venturing into our web frontend codebase.",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1",
"build": "tsc -p .",
"start": "npm run build -- -w"
},
"repository": {
"type": "git",
"url": "git+https://github.com/ThokozaniNqwili/onboard-javascript.git"
},
"author": "",
"license": "ISC",
"bugs": {
"url": "https://github.com/ThokozaniNqwili/onboard-javascript/issues"
},
"homepage": "https://github.com/ThokozaniNqwili/onboard-javascript#readme",
"devDependencies": {
"typescript": "^3.8.3"
},
"dependencies": {
"@types/jquery": "^3.5.16"
}
}
Loading