-
Notifications
You must be signed in to change notification settings - Fork 0
feature commit #2
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?
Conversation
vineet-suri
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.
Try to add comments to the functions wherever applicable
client/src/app.ts
Outdated
| } | ||
|
|
||
| export class User<T extends Array<object>,U extends Array<Array<string>>> implements UserAction{ | ||
| APIData:Array<object> = []; |
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.
user camel case for variables
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.
all variables are changed into camel case
client/src/app.ts
Outdated
| let val = descriptor.value | ||
| descriptor.value = function(... args:any[]){ | ||
| let arr:Array<string> = []; | ||
| for(let j=0;j<7;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.
try to use descriptive constants instead of numbers. That makes code more readable.
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.
changed the numbers into more readable format
client/src/models/model.ts
Outdated
| addUser(refer:any):void, | ||
| selectedRowEdit(refer:any):void, | ||
| selectedRowDelete(i:any):void |
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.
why using any type?
we should define proper types as far as possible.
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.
changed the any type with proper type
client/src/tabular.ts
Outdated
| } | ||
| let cell = row.insertCell(-1) | ||
| let inputField = document.createElement('input') as HTMLInputElement; | ||
| if(column==="Phone 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.
use enum for column names
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 reduce bugs due to typos
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.
changed the column name into enum
No description provided.