-
Notifications
You must be signed in to change notification settings - Fork 37
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
Complaints suggestions #49
base: master
Are you sure you want to change the base?
Complaints suggestions #49
Conversation
@preetamozarde3 check my review on #47; I had left 40 odd comments on it that'll need to be fixed. |
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 haven't tested this yet; but I'll try to do that in the next couple of days. In the meanwhile, other than the mostly minor issues I've pointed out below, you might want to make sure the design is responsive. Particularly, it should work on the following screen sizes (these are the sizes and browsers the rest of the app is being tested on):
- 4.7 inch phone (iPhone)
- 5.5+ inch phones (general android phones)
- 10 inch tablet (iPad, say)
- 14+ inch laptops
and must have browser compatibility for
- Firefox (Desktop)
- Chrome (Desktop)
- Edge (Desktop)
- Chrome (Android)
- Safari (iOS)
- IE 11 (best effort)
In general, you shouldn't have issues with JS compatibilty unless you're using some funky ES7 which you might need to polyfill, but CSS behavior can differ seriously between desktop browsers (which are somewhat consistent), Chrome for Android and iOS Safari.
Also, you could take cues from the rest of the app if you're unsure how this is supposed to look in responsive mode. In general, the screen is divided into three parts for desktop and tablet, with the navmenu at the left, a list or editing area at center and detailed information to the right. There are reusable components to make such a layout. More specifically, check the feed
, calendar
, add-event
and update-body
components' layouts.
src/app/app.module.ts
Outdated
@@ -100,6 +112,7 @@ import { CardComponent } from './card/card.component'; | |||
HttpClientModule, | |||
FormsModule, | |||
BrowserAnimationsModule, | |||
MyMaterialClass, |
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 already imported on line 157
src/app/app.module.ts
Outdated
@@ -120,7 +133,6 @@ import { CardComponent } from './card/card.component'; | |||
{ path: 'quick-links', component: QuickLinksComponent, data: { state: 'base' } }, | |||
{ path: 'settings', component: SettingsComponent, data: { state: 'base' } }, | |||
{ path: 'about', component: AboutComponent, data: { state: 'overlay' } }, | |||
|
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 whitespace was intentional
src/assets/placeholder_image.svg
Outdated
<g> | ||
<path d="M0,48v416h512V48H0z M480,432H32V80h448V432z M352,160c0,26.51,21.49,48,48,48s48-21.49,48-48s-21.49-48-48-48 | ||
S352,133.49,352,160z M448,400H64l96-256l128,160l64-48L448,400z"/> | ||
</g> |
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 add_image_placeholder.svg
(which already exists in assets) instead of adding a new file
src/app/app.module.ts
Outdated
], | ||
providers: [ | ||
DataService, | ||
{ provide: RouteReuseStrategy, useClass: CustomReuseStrategy }, | ||
LoginActivate, | ||
VenterDataService, |
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.
Move this just below DataService
src/app/interfaces.ts
Outdated
|
||
export interface IPostComment { | ||
text: 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.
You don't need separate interfaces for posting. Reuse IComplaintComment
.
subscribeToComplaint(complaint_subscribed: number, complaintId: string) { | ||
this.dataService.FireGET<IComplaint>(API.SubscribeToComplaint, | ||
{ complaintId: complaintId, action: complaint_subscribed === 0 ? 1 : 0 }).subscribe(result => { | ||
console.log(result); |
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 logging to console please.
} else if (complaint.status.toLowerCase() === 'resolved') { | ||
complaint.status_color = 'green'; | ||
} else if (complaint.status.toLowerCase() === 'in progress') { | ||
complaint.status_color = 'yellow'; |
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 switch...case here
if (this.complaint.description === '') { | ||
this.venterDataService.getSnackbar('Please enter a description before submitting the complaint!', null); | ||
} else { | ||
this.dataService.FirePOST<IComplaint>(API.Complaints, this.complaint).subscribe(() => {}); |
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.
Indent
this.editedComment = {} as IPostComment; | ||
/* Get profile if the user is logged in */ | ||
if (this.dataService.isLoggedIn()) { | ||
this.getUser(); |
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 still being treated as sync.
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 looked through this carefully; if I'm right that userid
isn't being used anywhere besides checking if the comment was made by the user, you could let this be the way it is and explicitly initialize userid
to null to be sure
}) | ||
export class ComplaintsHomeComponent implements OnInit { | ||
|
||
public complaints = {} as IComplaint[]; |
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're basically casting a JSON object to an array here, which works cause the way JS is, but this is terribly confusing. In general, it's better to initialize everything as null
(i.e. no initialization, just specify the type) and then put *ngIf
s to check if data has been loaded async.
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 tried that but I keep getting a TypeError (Cannot find property '...' of undefined)
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.
That means you're trying to access some member for the variable before it is initialized (probably a misplaced ngIf
). Before accessing anything in the variable, you need to check if it isn't null, easiest way to do which is add a *ngIf="complaints"
to the container in which it is being accessed.
mat-icon-button> | ||
<mat-icon>delete</mat-icon> | ||
</button> | ||
<button *ngIf="complaintComment.commented_by.id==userid" |
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 ==
|
||
public complaints = {} as IComplaint[]; | ||
public myComplaints = {} as IComplaint[]; | ||
public userid: 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.
Where is userid
being used in this component?
Here is an overview of what got changed by this pull request: Issues
======
- Added 140
See the complete overview on Codacy |
|
||
.complaint-tag { | ||
padding: 1% 0; | ||
background-color: yellowgreen |
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.
Issue found: Expected indentation of 2 spaces (indentation)
} | ||
|
||
.complaint-upvote-active { | ||
color: #536dfe; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
color: white; | ||
padding: 0 2.5%; | ||
font-weight: bold; | ||
text-transform: uppercase; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
|
||
.display-image { | ||
max-height: 400px; | ||
max-width: 400px; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
id: string; | ||
time: string; | ||
text: string; | ||
commented_by: IUserProfile; |
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.
Issue found: Identifier 'commented_by' is not in camel case.
|
||
.footer { | ||
position: relative; | ||
text-align: center; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
} | ||
|
||
.complaint-location-details { | ||
padding-top: 2%; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
|
||
.complaint-button { | ||
color: black; | ||
background-color: yellow; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
font-size: 18px; | ||
border-radius: 0 3px 3px 0; | ||
user-select: none; | ||
background-color: rgba(0,0,0,0.8); |
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.
|
||
.toolbar { | ||
padding: 150px 0; | ||
background-color: #536dfe; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
@@ -162,6 +162,52 @@ export interface IUserTagCategory { | |||
tags: IUserTag[]; | |||
} | |||
|
|||
export interface IComplaintTag { | |||
id: string; | |||
tag_uri: 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.
Issue found: Identifier 'tag_uri' is not in camel case.
|
||
.active, | ||
.dot:hover { | ||
background-color: #717171; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
/* The dots/bullets/indicators */ | ||
|
||
.dot { | ||
cursor: pointer; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
|
||
.dot { | ||
cursor: pointer; | ||
height: 15px; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
color: black; | ||
background-color: yellow; | ||
padding: 10px 50px; | ||
font-weight: bold; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
} | ||
|
||
.no-complaints-image { | ||
width: 50%; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
* {box-sizing: border-box} | ||
body {margin:0} | ||
.mySlides {display: none;} | ||
img {vertical-align: middle; height: 20%;} |
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.
* {box-sizing: border-box} | ||
body {margin:0} | ||
.mySlides {display: none;} | ||
img {vertical-align: middle; height: 20%;} |
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.
Issue found: Expected indentation of 2 spaces (indentation)
.status { | ||
float: right; | ||
background-color: red; | ||
border-width: medium; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
|
||
export interface IComplaint { | ||
id: string; | ||
created_by: IUserProfile; |
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.
Issue found: Identifier 'created_by' is not in camel case.
UserTagsReach: 'api/user-tags/reach' | ||
UserTagsReach: 'api/user-tags/reach', | ||
|
||
Complaints: 'api/venter/complaints', |
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.
Issue found: Strings must use doublequote.
margin: 0 2px; | ||
background-color: #bbb; | ||
border-radius: 50%; | ||
display: inline-block; |
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.
Issue found: Expected indentation of 2 spaces (indentation)
users_up_voted: IUserProfile[]; | ||
images: string[]; | ||
comments: IComplaintComment[]; | ||
vote_count: 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.
Issue found: Identifier 'vote_count' is not in camel case.
|
||
* {box-sizing: border-box} | ||
body {margin:0} | ||
.mySlides {display: none; padding-right: 5%;} |
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.
report_date: string; | ||
reported_date: string; | ||
status: string; | ||
status_color: 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.
Issue found: Identifier 'status_color' is not in camel case.
The following changes have been made in this branch: