-
Notifications
You must be signed in to change notification settings - Fork 62
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
fixes issue#925 #179
fixes issue#925 #179
Conversation
src/app/business/block/file-share-detail/file-share-detail.component.ts
Outdated
Show resolved
Hide resolved
src/app/business/block/file-share-detail/file-share-detail.component.ts
Outdated
Show resolved
Hide resolved
src/app/business/block/file-share-detail/file-share-detail.component.ts
Outdated
Show resolved
Hide resolved
LGTM |
@wisererik Please review. |
item.description = "--"; | ||
} | ||
item.date = new Date(Utils.formatDate(item.createdAt)).getTime(); | ||
item.description = (!item.description || item.description == '') ? '--' : item.description; |
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 consider making a reusable util method out of this
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.
@rhsakarpos I have added this to the backlog ( #186 ) since this is something that will have to be implemented at the application level. If this is agreeable please consider resolving this 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.
LG...AB
} | ||
this.acls.push(acl); | ||
} | ||
}) | ||
this.acls.sort((previous,later)=>{ |
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.
ideally, this is not needed, the table component in the UI should handle this.
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.
Since the table component is not used and the table is being generated with plain HTML and JS the sort method will have to be used. This can be looked into when we are redesigning the application.
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.
LG...AB
@rhsakarpos If the comments are not blockers consider LGTM. I will be taking over the development from here on on this and will consider your suggestions. |
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.
@rhsakarpos Please check my replies.
LG...AB |
Fixes sodafoundation/api#925