-
Notifications
You must be signed in to change notification settings - Fork 36
Onboarding Project: Maxwill #48
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?
Changes from 40 commits
3a4e462
8a555a7
75dc47a
476cad9
b11aad3
6d8b880
ecf387c
441ca1d
c948341
700d931
090f7cd
9005d45
86fb0dd
3b5990f
db01e42
2764976
df647ea
f8f8d24
44b81a7
ab121b8
0f4c00b
088d73b
975bc24
3fb7c44
ad77763
8704cb7
cff0b57
3198ed0
530cef4
48d8158
53ca9e5
765552d
59da1fc
d9882f8
18ee383
ff94cd8
99ca15c
12164f0
43bef10
9b9bcc6
e2e0439
1d6c7ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| /node_modules | ||
| app.js | ||
| app.js.map | ||
| apiManager.js | ||
| apiManager.js.map |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "version": "0.2.0", | ||
| "configurations": [ | ||
| { | ||
| "type": "chrome", | ||
| "request": "launch", | ||
| "name": "Debug in Chrome", | ||
| "url": "http://localhost:2050/", | ||
| "webRoot": "${workspaceFolder}", | ||
| "sourceMapPathOverrides": { | ||
| "webpack:///./*": "${webRoot}/*" | ||
| } | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "version": "2.0.0", | ||
| "tasks": [ | ||
| { | ||
| "type": "typescript", | ||
| "tsconfig": "tsconfig.json", | ||
| "option": "watch", | ||
| "problemMatcher": [ | ||
| "$tsc-watch" | ||
| ], | ||
| "group": "build", | ||
| "label": "tsc: watch - tsconfig.json" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,50 @@ | ||||||
| class ApiManager { | ||||||
|
|
||||||
| private mainUrl: string; | ||||||
|
|
||||||
| constructor(mainUrl: string) { | ||||||
|
Comment on lines
+2
to
+5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please leave an empty line between your class variables and |
||||||
| this.mainUrl = mainUrl; | ||||||
| } | ||||||
|
|
||||||
| private fetchJson(url: string): Promise<any> { | ||||||
| return fetch(url) | ||||||
| .then(res => { | ||||||
| if (res.ok) { | ||||||
| return res.json(); | ||||||
| } else { | ||||||
| throw new Error(`HTTP error! Status: ${res.status}`); | ||||||
| } | ||||||
| }) | ||||||
| .catch(error => { | ||||||
| throw new Error(`Fetch failed: ${error}`); | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| /** Retrieves records from the api */ | ||||||
| getRecords(fromID: number, toID: number): Promise<string[][]> { | ||||||
| return this.fetchJson(`${this.mainUrl}/records?from=${fromID}&to=${toID}`); | ||||||
| } | ||||||
|
|
||||||
| /** Retrieves columns from the api */ | ||||||
| getColumns(): Promise<string[]> { | ||||||
| return this.fetchJson(`${this.mainUrl}/columns`); | ||||||
| } | ||||||
|
|
||||||
| /** Retrieves the number of records there are */ | ||||||
| getRecordCount(): Promise<number> { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I seriously want to ask you to cache the record count result. It is the type of call you can make once and trust that it won't change again. However, there is nothing wrong with what you did, as this is a good approach in some cases. |
||||||
| return fetch(`${this.mainUrl}/recordCount`) | ||||||
| .then(res => { | ||||||
| if(res.ok) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return res.text(); | ||||||
| } else { | ||||||
| throw new Error(`HTTP error? Status: ${res.status}`); | ||||||
| } | ||||||
| }) | ||||||
| .then (recordCount => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return parseInt(recordCount); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would prefer it you checked for |
||||||
| }) | ||||||
| .catch(error => { | ||||||
| throw error; | ||||||
| }); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is missing some indentation. |
||||||
| } | ||||||
|
Comment on lines
+34
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So |
||||||
| } | ||||||
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.
Space inbetween.