-
Notifications
You must be signed in to change notification settings - Fork 1
fix: #90 Manage customer project types #152
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
Conversation
7d811c0 to
e88d1b9
Compare
...t/components/projects-type/components/create-project-type/create-project-type.component.html
Outdated
Show resolved
Hide resolved
|
|
||
| </div> | ||
| </form> | ||
| <hr /> |
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.
we can have a clean create-project-type.component if we move <app-project-type-list></app-project-type-list> to management-customer-projects.component.html. we can set in protect type tab the following:
<div class="container">
<app-create-project-type></app-create-project-type>
<app-project-type-list></app-project-type-list>
</div>
please remove <app-project-type-list></app-project-type-list> and <div class="container"> from here.
| <button type="button" class="btn btn-sm btn-secondary"><i class="fa fa-pencil fa-xs"></i></button> | ||
| <button type="button" class="btn btn-sm btn-danger ml-2"><i class="fas fa-trash-alt fa-xs"></i></button> | ||
| <button type="button" class="btn btn-sm btn-secondary"><i class="fa fa-pencil fa-xs" (click)="updateProjectType(projectType.id)"></i></button> | ||
| <button type="button" class="btn btn-sm btn-danger ml-2"><i class="fas fa-trash-alt fa-xs" (click)="deleteProjectType(projectType.id)" ></i></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 could change the click event to button label because the experience in the UI is uncomfortable. currently, you need to touch exactly over the icon for it works.
| import { ProjectTypeListComponent } from './project-type-list.component'; | ||
| import { provideMockStore, MockStore } from '@ngrx/store/testing'; | ||
| import { allProjectTypes, ProjectTypeState } from '../../store'; | ||
| import { NgxPaginationModule } from 'ngx-pagination'; |
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 sort this import.
| createProjectType(projectTypeData): Observable<any> { | ||
| const body = { | ||
| ...projectTypeData, | ||
| }; |
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 is not necessary if you don't need to add a new attribute.
| const url = `${this.baseUrl}/${projectTypeData.id}`; | ||
|
|
||
| const body = { | ||
| ...projectTypeData, |
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 is not necessary if you don't need to add a new attribute.
| import { ProjectType } from '../../../../shared/models'; | ||
|
|
||
| export enum ProjectTypeActionTypes { | ||
| LOAD_PROJECT_TYPES = '[projectType] LOAD_PROJECT_TYPES', |
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.
change all [projectType] to [ProjectType]
| export interface ProjectType { | ||
| id: string; | ||
| name: string; | ||
| description: 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.
description is optional, please add ? symbol.
| id: string; | ||
| name: string; | ||
| description: string; | ||
| tenant_id?: 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.
@enriquezrene I'm not sure if tenant_id still should be a property. We can get it from session storage and in this case, we don't use it
| rows="3" | ||
| placeholder="Description" | ||
| formControlName="description" | ||
| ></textarea> |
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.
we can set a default value for optional fields, that way we can avoid the error when we do two continue creations only with required fields
e88d1b9 to
1a2b4d5
Compare
1a2b4d5 to
5986a9f
Compare
Codecov Report
@@ Coverage Diff @@
## master #152 +/- ##
==========================================
+ Coverage 91.87% 92.40% +0.53%
==========================================
Files 50 53 +3
Lines 517 619 +102
Branches 32 36 +4
==========================================
+ Hits 475 572 +97
- Misses 33 37 +4
- Partials 9 10 +1
Continue to review full report at Codecov.
|
No description provided.