-
Notifications
You must be signed in to change notification settings - Fork 35
Ashton Martin #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Ashton Martin #45
Conversation
app.ts
Outdated
| } else { | ||
| //pass | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the else
app.ts
Outdated
| } else { | ||
| //pass | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the else
app.ts
Outdated
| } else { | ||
| //pass | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the else
app.ts
Outdated
| } else { | ||
| //pass | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the else.
app.ts
Outdated
| } else { | ||
| //pass | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the else.
app.ts
Outdated
| } else { | ||
| //pass | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the else.
app.ts
Outdated
| let fromNumber: any; | ||
| let toNumber: any; | ||
| let recordDiff: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use any if not necessary. Give them a specific type.
app.ts
Outdated
| headers: { "Content-Type": "application/json" }, | ||
| }) | ||
| .then((response) => response.text()) | ||
| .then((data) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a catch to log and handle errors.
app.ts
Outdated
| } | ||
| ) | ||
| .then((response) => response.text()) | ||
| .then((data) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a catch to log and handle errors.
|
|
||
| var columns = [13]string{"ID", "A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L"} | ||
| const recordCount = 1000000 | ||
| const columnCount = 11 | ||
| const columnCount = len(columns); | ||
| const delayResponse = 500 * time.Millisecond | ||
|
|
||
| var columns = [columnCount]string{"ID", "A", "B", "C", "D", "E", "F", "G", "H", "I", "J"} | ||
| var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't touch the backend. Frontend only.
app.ts
Outdated
| @@ -0,0 +1,414 @@ | |||
| // Get the container from the html where the data will be stored | |||
| let recordNav: any = document.querySelector("#record-navigation-container"); //Navigation area; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These do not need to be global variables. Global variables are generally frowned upon. There are a few acceptable global variables but try to remove these from being global.
app.ts
Outdated
| } | ||
|
|
||
| // The functionality to move through an amount of records. | ||
| function nextPrevious(firstId: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the any type sparingly. Go through the JavaScript Style Guide to see preferences and standards.
app.ts
Outdated
| let amountRecord: any; | ||
|
|
||
| if (finalId >= 999999) { | ||
| if (window.innerHeight > 500) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid doing this check.Try to fit as much readable rows on the screen as possible. Screen sizes will vary so rather try to have a function that calculates and returns the number of rows you can fit on the page and then use that value to set your parameters for fetching the table data.
app.ts
Outdated
| finalId = 999999; | ||
| firstId = finalId - 2; | ||
| } else if (window.innerHeight <= 170) { | ||
| finalId = 999999; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finalId is being repeated multiple times in the if statements. This is a good sign to either create a method or just simply move that line outside of the if-else statements. Code refactoring basically :P
app.ts
Outdated
| } else { | ||
| if (window.innerHeight > 500) { | ||
| amountRecord = 14; | ||
| finalId = firstId + amountRecord; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more repetitive code here.
app.ts
Outdated
| (finalId - firstId + 1) + | ||
| " records only."; | ||
|
|
||
| console.log(typeof firstId, typeof finalId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this was for testing your code? You can remove any console.logs if they are not necessary.
app.ts
Outdated
|
|
||
| finalId = 999999; | ||
|
|
||
| if (window.innerHeight > 500) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checking for the screen size is being done for a second time (similar to earlier in the nextPrevious() function), so I think put this logic into its own method which you can call when you need to know whats the number of rows needed for the current screen size.
app.ts
Outdated
| } | ||
|
|
||
| let resizeScreen = () => { | ||
| if (window.innerHeight > 500) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This window size check is repeated here again.
index.html
Outdated
| <div id="record-navigation-container"></div> | ||
| <div id="column-headings-container"></div> | ||
| <div id="info-columns-container"></div> | ||
| <script src="./app.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move your script to the head tag.
index.html
Outdated
| <script | ||
| type="text/javascript" | ||
| charset="utf-8" | ||
| src="third_party/jquery-2.0.3.min.js" | ||
| ></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather keep this to one line.
| <script | |
| type="text/javascript" | |
| charset="utf-8" | |
| src="third_party/jquery-2.0.3.min.js" | |
| ></script> | |
| <script type="text/javascript" charset="utf-8" src="third_party/jquery-2.0.3.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
app.ts
Outdated
| let recordNav: any = document.querySelector("#record-navigation-container"); //Navigation area; | ||
| let headingColumns: any = document.querySelector("#column-headings-container"); //Headings; | ||
| let infoColumns: any = document.querySelector("#info-columns-container"); //Information; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use global variables, rather move them to a class (denounce is fine tho).
app.ts
Outdated
| if (firstId === 0) { | ||
| previousBtn.disabled = true; | ||
| nextBtn.disabled = false; | ||
| } else if (finalId === 999999) { | ||
| nextBtn.disabled = true; | ||
| previousBtn.disabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use hard coded values (i.e. 0 and 999999) to do checks on your ids. What would happen if a data set of a different size is given? Try to keep checks on your data as generic as possible.
anzelIMQS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more little changes.
app.ts
Outdated
| firstId = firstId; | ||
| finalId = finalId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It surely shouldn't be necessary to set these variables to themselves? Especially since they are global variables in your case.
app.ts
Outdated
| currentPage.innerHTML = | ||
| firstId + | ||
| " / " + | ||
| finalId + | ||
| " " + | ||
| (finalId - firstId + 1) + | ||
| " records only."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rather make use of template literals here.
It's better suited for string concatenation when you have more than 1 variable in your string.
anzelIMQS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do check some of the comments I made and see if they are applicable somewhere else where I might have missed them.
Please do ask me if something is unclear in what I said.
app.ts
Outdated
| headers: { "Content-Type": "application/json" }, | ||
| }) | ||
| .then((response) => response.text()) | ||
| .then((data) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement the catch on the Promise. .then().catch( () => {});
app.ts
Outdated
| let tryCatch = (func: any) => { | ||
| try { | ||
| func; | ||
| } catch (error) { | ||
| console.log(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not do it this way but rather on the Promise.
server/main.go
Outdated
|
|
||
| const recordCount = 1000000 | ||
| const columnCount = 11 | ||
| const columnCount = 11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the whitespace
app.ts
Outdated
|
|
||
| function recordSelection() { | ||
| let selectionArea: any = document.querySelector("#record-navigation-container"); | ||
| let SingleRecordSelection = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make it lowerCamelCase
app.ts
Outdated
| currentPage.innerHTML = ""; | ||
| currentPage.innerHTML = currentPage.innerHTML = fromNumber + " / " + toNumber + " " + (Number(numberOfRows) + 1) + " records only."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to clear it. If you set it a value, the previous values are overwritten.
app.ts
Outdated
|
|
||
| let nextPrevious = (fromNumber: number) => { | ||
| let numberOfRows = Math.floor(window.innerHeight / 50); | ||
| let count: number = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove count. No need to multiply. The next function is supposed to be executed every time it passes debouncing.
app.ts
Outdated
| } | ||
| } | ||
|
|
||
| let nextPrevious = (fromNumber: number) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably give this function a more descriptive name
app.ts
Outdated
| previousPage = debounce(previousPage, 500); | ||
| previousBtn.addEventListener("click", () => { | ||
| count++; | ||
| previousPage(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass in directly.
app.ts
Outdated
| nextPage = debounce(nextPage, 500); | ||
| nextBtn.addEventListener("click", () => { | ||
| count++; | ||
| nextPage(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass in directly
app.ts
Outdated
| let headingColumns: any = document.querySelector("#column-headings-container"); //Headings; | ||
| let infoColumns: any = document.querySelector("#info-columns-container"); //Information; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add whitespace between // and comment. Can probably remove the semi-colon as well.
anzelIMQS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to make use of the API recordCount. Also, remove some redundancy.
app.ts
Outdated
| }; | ||
|
|
||
| lastPageBtn.addEventListener("click", () => { | ||
| console.log("Hello"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove fancy logging
app.ts
Outdated
| }; | ||
|
|
||
| function getRecords(fromNumber: number, toNumber: number) { | ||
| fetch("http://localhost:2050/records?from=" + fromNumber + "&to=" + toNumber, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this Template literals.
app.ts
Outdated
| } else if (typeof fromNumber === "number" && fromNumber >= 0) { | ||
| getRecords(fromNumber, toNumber); | ||
| } else { | ||
| alert("Error"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tell me what's wrong? Error what?
app.ts
Outdated
| selectionArea.innerHTML = ""; | ||
| selectionArea.innerHTML = singleRecordSelection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not necessary to clear it since you are anyway assigning it a new value that overwrites the previous one?
app.ts
Outdated
| nextBtn.disabled = false; | ||
| previousBtn.disabled = false; | ||
|
|
||
| if (fromNumber + numberOfRows >= 999999) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only noticed now you are working on a hardcoded value for the max recordCount (999999). Please ask the API once for that value and store it. You can then use it accordingly instead of the working of a hardcoded values.
| const columnCount = 11 | ||
| const delayResponse = 500 * time.Millisecond | ||
|
|
||
| var columns = [columnCount]string{"ID", "A", "B", "C", "D", "E", "F", "G", "H", "I", "J"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be any changes to this file. Looks like you accidentally removed the new lines. Make sure this file doesn't even popup on Files changed on GitHub.
FritzOnFire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please convert your spaces into tabs.
Please remove the text file from the PR.
Please remove all the any types. The only place I know of that you would need an any is when getting data from the back-end, but most people just specify the actual type there anyway.
You have a lot of global variables and logic being executed in global scope. Please move them into a class, or somewhere where they are no longer considered to be global. (Hint, the class option will ensure that you get way less future review changes)
app.ts
Outdated
| let headingColumns: any = document.querySelector("#column-headings-container"); // Headings | ||
| let infoColumns: any = document.querySelector("#info-columns-container"); // Information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way your code is written these lines will execute before those elements actually exist on the webpage. So they can definitely not be here. You should only try and get things from the webpage after it has been rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also change the any to the actual type.
| "strconv" | ||
| "time" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not show up in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
app.ts
Outdated
| <button value="previous" class="previous-records-btn">Previous</button> | ||
| <button value="next" class="next-records-btn">Next</button> | ||
| <button value="last" class="last-page-btn">Last Page</button> | ||
| <button onclick="recordSelection()" id="confirmation-btn">Get Record</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not specify your onclick in the HTML. You should do it in the typescript code.
app.ts
Outdated
| }; | ||
| }; | ||
|
|
||
| let createNavigation = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this variable to a normal function, seeing as that you never override the value.
app.ts
Outdated
| type="text" | ||
| min="0" | ||
| minlength="1" | ||
| maxlength="6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding the max length here is a bad idea, as the maximum amount of rows that you could get is unknown.
app.ts
Outdated
| } | ||
| resizeScreenData(); | ||
| }) | ||
| .catch((error) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the () around the error parameter or specify the type, as the compiler currently thinks it is of type any.
app.ts
Outdated
| }; | ||
|
|
||
| function getRecords(fromNumber: number, toNumber: number) { | ||
| fetch(`http://localhost:2050/records?from=${fromNumber}&to=${toNumber}`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a return in front of your fetch to allow other functions to wait on this function.
app.ts
Outdated
| .then((response) => response.text()) | ||
| .then((data) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the () around the parameters or specify the types, as the compiler currently thinks they are of type any.
app.ts
Outdated
| method: "GET", | ||
| headers: { "Content-Type": "application/json" }, | ||
| }) | ||
| .then((response) => response.text()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should check if response has an error here.
app.ts
Outdated
| let currentPage: any = document.querySelector(".current-page"); | ||
| currentPage.innerHTML = `${fromNumber} / ${toNumber}.`; | ||
| }) | ||
| .catch((error) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the () around the error parameter or specify the type, as the compiler currently thinks it is of type any.
FritzOnFire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the spaces to tabs.
| fromNumber = 0; | ||
| }; | ||
|
|
||
| function checkResponseError(response: Response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify a return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
app.ts
Outdated
| } | ||
| } | ||
|
|
||
| function getRecords(fromNumber: number, toNumber: number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify a return type
app.ts
Outdated
| headers: { "Content-Type": "application/json" }, | ||
| }) | ||
| .then(checkResponseError) | ||
| .then((response: Response) => response.text()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should rather use .json(). Then you don't have to parse the result and check if it successfully parsed in the next section.
app.ts
Outdated
| }) | ||
| .then(checkResponseError) | ||
| .then((response: Response) => response.text()) | ||
| .then((data) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either remove the () or specify the type, as the compiler currently thinks the type is any.
app.ts
Outdated
| let infoColumns: HTMLElement | null = document.getElementById("info-columns-container"); // Information | ||
|
|
||
| console.log("Hello"); | ||
| (infoColumns as HTMLDivElement).innerHTML = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to check if infoColumns is null, and handle it appropriately. Instead of hard casting the value.
app.ts
Outdated
| let nextBtn: HTMLElement | null = document.getElementById("next-records-btn"); | ||
| let previousBtn: HTMLElement | null = document.getElementById("previous-records-btn"); | ||
| let navBtns = document.getElementById("navigation-btns"); | ||
| if (recordNav !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably error or something if this happens, otherwise the user will not know whats wrong.
app.ts
Outdated
| (nextBtn as HTMLButtonElement).disabled = false; | ||
| (previousBtn as HTMLButtonElement).disabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should check for nulls.
app.ts
Outdated
| recordCount(); | ||
| window?.addEventListener("resize", debounce(resizeScreenData, 500)); | ||
| createNavigation(); | ||
| headingRowCreation(); | ||
|
|
||
| { | ||
| let confirmationBtn: HTMLElement | null = document.getElementById("confirmation-btn"); | ||
| confirmationBtn?.addEventListener("click", recordSelection); | ||
|
|
||
| let nextBtn: HTMLElement | null = document.getElementById("next-records-btn"); | ||
| let previousBtn: HTMLElement | null = document.getElementById("previous-records-btn"); | ||
| let firstPageBtn: HTMLElement | null = document.getElementById("first-page-btn"); | ||
| let lastPageBtn: HTMLElement | null = document.getElementById("last-page-btn"); | ||
|
|
||
| let nextPage = () => { | ||
| console.log(fromNumber); | ||
| let numberOfRows = Math.floor(window.innerHeight / 50); | ||
| fromNumber = fromNumber + numberOfRows * count; | ||
| let toNumber = fromNumber + numberOfRows; | ||
|
|
||
| let finalRecord = recordNumberTotal - 1; | ||
|
|
||
| (previousBtn as HTMLButtonElement).disabled = false; | ||
|
|
||
| if (toNumber >= finalRecord) { | ||
| (nextBtn as HTMLButtonElement).disabled = true; | ||
| fromNumber = finalRecord - numberOfRows; | ||
| } | ||
|
|
||
| getRecords(fromNumber, toNumber); | ||
| count = 0; | ||
| }; | ||
| nextBtn?.addEventListener("click", () => { | ||
| count++; | ||
| nextPage = debounce(nextPage, 500); | ||
| nextPage(); | ||
| }); | ||
|
|
||
| let previousPage = () => { | ||
| let numberOfRows = Math.floor(window.innerHeight / 50); | ||
| fromNumber = fromNumber - numberOfRows * count; | ||
| let toNumber = fromNumber + numberOfRows; | ||
| (nextBtn as HTMLButtonElement).disabled = false; | ||
|
|
||
| if (fromNumber <= 0) { | ||
| (previousBtn as HTMLButtonElement).disabled = true; | ||
| fromNumber = 0; | ||
| } | ||
|
|
||
| getRecords(fromNumber, toNumber); | ||
| count = 0; | ||
| }; | ||
| previousBtn?.addEventListener("click", () => { | ||
| count++; | ||
| previousPage = debounce(previousPage, 500); | ||
| previousPage(); | ||
| }); | ||
|
|
||
| let firstPage = () => { | ||
| fromNumber = 0; | ||
| let numberOfRows = Math.floor(window.innerHeight / 50); | ||
| let toNumber = fromNumber + numberOfRows; | ||
| (nextBtn as HTMLButtonElement).disabled = false; | ||
| (previousBtn as HTMLButtonElement).disabled = true; | ||
|
|
||
| getRecords(fromNumber, toNumber); | ||
| }; | ||
| firstPageBtn?.addEventListener("click", () => { | ||
| firstPage(); | ||
| }); | ||
|
|
||
| let lastPage = () => { | ||
| let finalRecord = recordNumberTotal - 1; | ||
| let numberOfRows = Math.floor(window.innerHeight / 50); | ||
| fromNumber = finalRecord - numberOfRows; | ||
| (nextBtn as HTMLButtonElement).disabled = true; | ||
| (previousBtn as HTMLButtonElement).disabled = false; | ||
|
|
||
| getRecords(fromNumber, finalRecord); | ||
| }; | ||
| lastPageBtn?.addEventListener("click", () => { | ||
| lastPage(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to much logic happening in global space, please either move it into a class, or have it executed somewhere else.
index.html
Outdated
| <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="./app.js" defer></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not really be using defer for this project... but there aren't any rules against it, so I can't ask you to remove it.
| "strconv" | ||
| "time" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
FritzOnFire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debounces or throttles may not exceed 250ms, ideally the closer to 50ms the better. Please change yours, 500ms is way to much.
app.ts
Outdated
| return response; | ||
| } | ||
|
|
||
| function debounce(func: any, delay: number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify the type of func. Im pretty sure it should be () => void.
app.ts
Outdated
| }) | ||
| .then(checkResponseError) | ||
| .then((response: Response) => response.json()) | ||
| .then((data: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this is a string, and then a few lines down you have a for-loop over it?
app.ts
Outdated
| createNavigation(); | ||
| recordCount(); | ||
| headingRowCreation(); | ||
| fromNumber = 0; | ||
| new pageNavigation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should create the functions and classes before you use them (They should appear before the window.onload line)
app.ts
Outdated
| let currentPage: HTMLElement | null = document.getElementById("current-page"); | ||
|
|
||
| if (infoColumns !== null) { | ||
| (infoColumns as HTMLDivElement).innerHTML = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to hard cast to HTMLDivElement if you have checked for the null. Please remove it.
app.ts
Outdated
| } | ||
|
|
||
| if (currentPage !== null) { | ||
| (currentPage as HTMLParagraphElement).innerHTML = `${fromNumber} / ${toNumber}.`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the comment before, please remove the hard cast.
app.ts
Outdated
| (this.nextBtn as HTMLButtonElement).disabled = true; | ||
| (this.previousBtn as HTMLButtonElement).disabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment about these two buttons.
index.html
Outdated
| <script | ||
| type="text/javascript" | ||
| charset="utf-8" | ||
| src="third_party/jquery-2.0.3.min.js" | ||
| ></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
index.html
Outdated
| <div id="column-headings-container"></div> | ||
| <div id="info-columns-container"></div> | ||
| </body> | ||
| <script src="./app.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line needs to remain in your head tag.
index.html
Outdated
| <p>Hello</p> | ||
| </body> | ||
|
|
||
| <head> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please convert this file back to using tabs.
style.css
Outdated
| @@ -0,0 +1,116 @@ | |||
| * { | |||
| margin: 0; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please convert this file to use tabs
FritzOnFire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather never use the ! to indicate that something is not null. It will really bite you in the future and cause a lot of extra work.
| fromNumber = 0; | ||
| }; | ||
|
|
||
| function checkResponseError(response: Response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
app.ts
Outdated
| }) | ||
| .then(checkResponseError) | ||
| .then((response: Response) => response.json()) | ||
| .then((data: TemplateStringsArray) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .then((data: TemplateStringsArray) => { | |
| .then((data: string[][]) => { |
app.ts
Outdated
| returnBtn.addEventListener("click", () => { | ||
| createNavigation(); | ||
| recordCount(); | ||
| resizeScreenData() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing ;
app.ts
Outdated
| recordCount(); | ||
| resizeScreenData() | ||
| fromNumber = 0; | ||
| getRecords(fromNumber, fromNumber + numberOfRows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing ;
app.ts
Outdated
| recordCount(); | ||
| resizeScreenData() | ||
| fromNumber = 0; | ||
| getRecords(fromNumber, fromNumber + numberOfRows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have no guarentee that recordCount will finish before getRecords is called. You should change this to only call getRecords if recordCount has finished.
app.ts
Outdated
| nextBtn!.disabled = false; | ||
| previousBtn!.disabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know if these two buttons are not null?
app.ts
Outdated
| nextBtn!.disabled = true; | ||
| previousBtn!.disabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know if these two buttons are not null?
app.ts
Outdated
| nextBtn!.disabled = false; | ||
| previousBtn!.disabled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know if these two buttons are not null?
app.ts
Outdated
| }) | ||
| .then(checkResponseError) | ||
| .then((response: Response) => response.json()) | ||
| .then((data: TemplateStringsArray) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .then((data: TemplateStringsArray) => { | |
| .then((data: string[]) => { |
app.ts
Outdated
| function navigationDebounce(func: (argOne: number, argTwo: number) => void, delay: number) { | ||
| return function (argOne: number, argTwo: number) { | ||
| clearTimeout(timeout); | ||
|
|
||
| timeout = setTimeout(() => { | ||
| func(argOne, argTwo); | ||
| }, delay); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this into the class. The more things you put in a class the better.
Onboarding JS Project