fix: #371 Add_confirmation_before_deleting_data #387
Conversation
Codecov Report
@@ Coverage Diff @@
## master #387 +/- ##
==========================================
- Coverage 91.72% 90.40% -1.32%
==========================================
Files 71 72 +1
Lines 1184 1220 +36
Branches 77 78 +1
==========================================
+ Hits 1086 1103 +17
- Misses 71 89 +18
- Partials 27 28 +1
Continue to review full report at Codecov.
|
| fixture.detectChanges(); | ||
| component.removeListById(merged.id); | ||
| expect(component.removeList.emit).toHaveBeenCalled(); | ||
| component.cancelDeleteModal.nativeElement.click(); |
There was a problem hiding this comment.
What is this line for?
There was a problem hiding this comment.
this line is to close de modal
| </button> | ||
| </div> | ||
| <div class="modal-body"> | ||
| Are you sure you want to delete <b>{{ list.project_name || list.name }}</b> |
There was a problem hiding this comment.
Don't we need a question mark at the end of this sentence?
There was a problem hiding this comment.
We do need it for sure. So messages should look like:
Are you sure you want to delete XYZ?
There was a problem hiding this comment.
yes this is working, I need to put this {{ list.project_name || list.name }} because the objects don't have the same names.
| Are you sure you want to delete <b>{{ list.project_name || list.name }}</b> | ||
| </div> | ||
| <div class="modal-footer"> | ||
| <button (click)="removeListById(list.id)" type="button" class="btn btn-primary">Delete</button> |
There was a problem hiding this comment.
Since this is a confirmation dialog, I would change the Delete/Cancel button texts by Yes/No or Confirm/Cancel so that in the future we can reuse this confirmation dialog in any other action that demands user confirmation.
There was a problem hiding this comment.
Probably not, since this component is intended to be used just to confirm delete actions. In this sense, I'd rather call it
src/app/modules/shared/components/dialogs/remove.dialog.component.html
The functionality works good but we need to move the buttons so the first option is Cancel and the second one is Delete. This is how confirmation modal panels usually look like:
Taking the image into account, let's use the style danger for the Delete button.
| <div class="modal-dialog" role="document"> | ||
| <div class="modal-content" *ngIf="list"> | ||
| <div class="modal-header"> | ||
| <h5 class="modal-title" id="exampleModalLabel">{{ title }}</h5> |
There was a problem hiding this comment.
id would me modalTitle or so. Please replace exampleModalPanel
There was a problem hiding this comment.
Any chance to put the title using a bold style?
| </button> | ||
| </div> | ||
| <div class="modal-body"> | ||
| Are you sure you want to delete <b>{{ list.project_name || list.name }}</b> |
There was a problem hiding this comment.
We do need it for sure. So messages should look like:
Are you sure you want to delete XYZ?
| Are you sure you want to delete <b>{{ list.project_name || list.name }}</b> | ||
| </div> | ||
| <div class="modal-footer"> | ||
| <button (click)="removeListById(list.id)" type="button" class="btn btn-primary">Delete</button> |
There was a problem hiding this comment.
Probably not, since this component is intended to be used just to confirm delete actions. In this sense, I'd rather call it
src/app/modules/shared/components/dialogs/remove.dialog.component.html
The functionality works good but we need to move the buttons so the first option is Cancel and the second one is Delete. This is how confirmation modal panels usually look like:
Taking the image into account, let's use the style danger for the Delete button.
DiegoTinitana
force-pushed
the
371_Add_confirmation_before_deleting_data
branch
3 times, most recently
from
fa8fe33 to
3a80c14
Compare
| </div> | ||
| <div class="modal-footer"> | ||
| <button #cancelDeleteModal type="button" class="btn btn-secondary" data-dismiss="modal">Cancel</button> | ||
| <button (click)="closeModal()" type="button" class="btn btn-danger">Delete</button> |
There was a problem hiding this comment.
Replace Delete by Confirm so that this modal can be used by other confirm scenarios
DiegoTinitana
force-pushed
the
371_Add_confirmation_before_deleting_data
branch
from
3a80c14 to
2c9c0a2
Compare
No description provided.