mgr/dashboard: Adds reusable deletion dialog #20899

Devp00l · 2018-03-14T15:55:00Z

You can now simply use a deletion dialog without having to import a lot
of different things from ngx-bootstrap. Its easy to extend the dialog
by a detail description.

Signed-off-by: Stephan Müller smueller@suse.com

This commit is based on two commits from @tspmelo.

To see a working example look here

The updated example on how to use the different deletion methods can be found here

Devp00l · 2018-03-14T16:02:25Z

This animation was created using the other branch named "deletion-dialog-example".

votdev · 2018-03-23T09:30:30Z

Could you please rebase this PR.

ricardoasmarques

Shouldn't we wait for #21011 ?

ricardoasmarques · 2018-03-23T17:12:32Z

.../mgr/dashboard/frontend/src/app/shared/components/deletion-modal/deletion-modal.component.ts

+export class DeletionModalComponent implements OnInit {
+  @Input() metaType: string;
+  @Input() pattern: string;
+  @Input() btnClasses = 'btn btn-primary';


Should be 'btn btn-sm btn-primary' (dashboard buttons are small).

This is correct, but the button still does not fit in a btn-group. I think we need to duplicate the btn-group CSS for cd-deletion-modal.

ricardoasmarques · 2018-03-23T17:15:30Z

...gr/dashboard/frontend/src/app/shared/components/deletion-modal/deletion-modal.component.html

+    </ng-container>
+
+    <ng-container class="modal-footer">
+      <button type="button"


Shouldn't we use a "cd-submit-button" here (after #21011 is merged)?

With a "cd-submit-button" you will have auto focus on invalid field.

ricardoasmarques · 2018-03-23T17:31:10Z

...gr/dashboard/frontend/src/app/shared/components/deletion-modal/deletion-modal.component.html

+</ng-template>
+
+<ng-template #deletionButtonLabel>
+  <i class="fa fa-fw fa-trash-o"></i>


I'm sure we should use an icon on the "first" button, but IMO we don't need this icon on the modal "submit" button because we don't use icons on any other form submission (e.g. Pool creation, RBD creation).

votdev · 2018-03-26T07:52:34Z

I do not like that the component does not take care about showing a progress spinner somewhere until the deletion is done. The current implementation forces the developer to do this on its own.

votdev · 2018-03-26T07:49:11Z

.../mgr/dashboard/frontend/src/app/shared/components/deletion-modal/deletion-modal.component.ts

+import { BsModalRef, BsModalService } from 'ngx-bootstrap';
+
+@Component({
+  selector: 'cd-deletion-modal',


Maybe consider to rename it to something like cd-deletion-button.

votdev · 2018-03-26T07:56:19Z

.../mgr/dashboard/frontend/src/app/shared/components/deletion-modal/deletion-modal.component.ts

+    }
+  }
+
+  openModal(template: TemplateRef<any>) {


I think we should name this method showModal.

votdev · 2018-03-26T13:58:26Z

src/pybind/mgr/dashboard/frontend/src/app/shared/components/modal/modal.component.ts

+
+  constructor(private modalService: BsModalService) {}
+
+  openModal(template: TemplateRef<any>) {


Consider renaming this method to showModal.

ricardoasmarques · 2018-04-09T12:19:45Z

Delete confirmation dialog does not close automatically after submit, is this something we should do manually? If yes, how should we implement it?

Devp00l · 2018-04-09T13:24:26Z

@ricardoasmarques it closes automatically if you provide a delete method that is a observable - If you don't you have to close it manually - You can see how each way can be done in this example.

The problem is if the delete function is only triggered though emit the modal won't know when the deletion is done and therefore should not close itself.

ricardoasmarques · 2018-04-09T13:37:31Z

@Devp00l The required validation message is not displayed when I click on Delete button before I type anything on the input (I mean, if input is not dirty). Is this something that can be fixed?

votdev · 2018-04-10T09:41:21Z

src/pybind/mgr/dashboard/frontend/src/app/shared/components/modal/modal.component.ts

+
+  constructor(private modalService: BsModalService) {}
+
+  openModal(template: TemplateRef<any>) {


This method is not used in this PR, so it can be removed, right?

tspmelo · 2018-04-09T16:51:00Z

.../dashboard/frontend/src/app/shared/components/deletion-button/deletion-button.component.html

+  </cd-modal>
+</ng-template>
+
+<ng-template #deletionButtonLabel>


This is not being used.

tspmelo · 2018-04-10T10:16:07Z

.../dashboard/frontend/src/app/shared/components/deletion-button/deletion-button.component.html

+  <ng-container *ngTemplateOutlet="deletionHeading"></ng-container>
+</ng-template>
+
+<ng-template #dialogDeleteButton>


I would suggest we don't create a template for elements that are only used once.
cdSubmitButton can be inserted directly in the the modal-footer element.

It's only possible to use the submit button with the right scope in the modal if its a template.

tspmelo · 2018-04-10T10:16:32Z

.../dashboard/frontend/src/app/shared/components/deletion-button/deletion-button.component.html

+  {{ metaType }}
+</ng-template>
+
+<ng-template #deletionDescription>


Same situation as cdSubmitButton.

votdev · 2018-04-11T08:55:10Z

...gr/dashboard/frontend/src/app/shared/components/deletion-button/deletion-button.component.ts

+export class DeletionButtonComponent implements OnInit {
+  @ViewChild(SubmitButtonComponent) submitButton: SubmitButtonComponent;
+  @Input() metaType: string;
+  @Input() pattern: string;


Does it make sense to set a default value here?

tspmelo · 2018-04-11T09:00:40Z

...gr/dashboard/frontend/src/app/shared/components/deletion-button/deletion-button.component.ts

+  }
+
+  invalidControl (control: FormControl, error?: string) {
+    return control.dirty && control.invalid && (error ? control.errors[error] : true);


We should also check if the form is in the submitted state. something like (formDir.submitted || control.dirty) && ..

Devp00l · 2018-04-16T09:20:59Z

Changed the button into a link - now you can use it everywhere :)

votdev · 2018-04-16T10:03:24Z

.../mgr/dashboard/frontend/src/app/shared/components/deletion-link/deletion-link.component.html

+  <ng-container i18n>
+    Delete
+  </ng-container>
+  {{ metaType }}


Shouldn't this be translated?

votdev · 2018-04-16T10:03:50Z

.../mgr/dashboard/frontend/src/app/shared/components/deletion-link/deletion-link.component.html

+        </span>
+        <span class="help-block"
+              *ngIf="invalidControl(formDir.submitted, 'pattern')">
+          '{{ confirmation.value }}'


Shouldn't this be translated?

votdev · 2018-04-16T10:03:57Z

.../mgr/dashboard/frontend/src/app/shared/components/deletion-link/deletion-link.component.html

+        <ng-container i18n>
+          To confirm the deletion, enter
+        </ng-container>
+        <kbd>{{ pattern }}</kbd>


Shouldn't this be translated?

votdev · 2018-04-16T10:14:16Z

Dropdown looks now like

ricardoasmarques · 2018-04-16T13:53:10Z

Per our discussion today, this PR should only include a component that represents the modal, and each developer is responsible for opening and closing that modal.

After RBD management PR is merged, you should addapt your code based on mgr/dashboard: Add 'delete-confirmation-modal' component commit -
https://github.com/ricardoasmarques/ceph/commits/wip-list-all-rbds

ricardoasmarques · 2018-04-19T14:43:07Z

'Cancel' is not working for the deletion modal.

When I try to cancel the deletion modal, a validation error is displayes instead of closing the moda:

Devp00l · 2018-04-20T14:21:23Z

Addressed all commits, added a setup method that check that everything needed is set. Also I now mocked show form modalService to prevent detached modals on the chrome output.

ricardoasmarques · 2018-04-23T09:57:17Z

If we receive an error from server, the Delete button should be re-enabled

Doing something like this.deletionForm.setErrors({'cdSubmitButton': true}); should fix the problem.

votdev · 2018-04-23T14:14:19Z

.../mgr/dashboard/frontend/src/app/shared/components/deletion-modal/deletion-modal.component.ts

+} from '@angular/core';
+import { FormControl, FormGroup, Validators } from '@angular/forms';
+
+import { BsModalRef, BsModalService } from 'ngx-bootstrap';


BsModalService can be removed because it is not used.

Devp00l · 2018-04-23T16:59:36Z

@ricardoasmarques Added the error to the stopLoadingSpinner method which you have to call manually if you don't use an observable deletion method.

@votdev removed the unused import

ricardoasmarques · 2018-04-24T09:03:23Z

@Devp00l can you please apply the following patch to guarantee that we stop the spinner button on RBD dialogs:

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-list/rbd-list.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-list/rbd-list.component.ts
index ee4b58e9c0..6ac2fbb02a 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-list/rbd-list.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-list/rbd-list.component.ts
@@ -235,6 +235,7 @@ export class RbdListComponent implements OnInit, OnDestroy {
         this.modalRef.hide();
         this.loadImages(null);
       }).catch((resp) => {
+        this.modalRef.content.stopLoadingSpinner();
         finishedTask.success = false;
         finishedTask.exception = resp.error;
         this.notificationService.notifyTask(finishedTask);
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.ts
index 52752febf4..270822bee1 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-snapshot-list/rbd-snapshot-list.component.ts
@@ -207,7 +207,7 @@ export class RbdSnapshotListComponent implements OnInit, OnChanges {
           });
       })
       .catch((resp) => {
-        this.modalRef.hide();
+        this.modalRef.content.stopLoadingSpinner();
         finishedTask.success = false;
         finishedTask.exception = resp.error;
         this.notificationService.notifyTask(finishedTask);
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rollback-confirmation-modal/rollback-confimation-modal.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rollback-confirmation-modal/rollback-confimation-modal.component.ts
index 596c6e9427..b95d7e4704 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rollback-confirmation-modal/rollback-confimation-modal.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rollback-confirmation-modal/rollback-confimation-modal.component.ts
@@ -32,4 +32,8 @@ export class RollbackConfirmationModalComponent implements OnInit {
   submit() {
     this.onSubmit.next(this.snapName);
   }
+
+  stopLoadingSpinner() {
+    this.rollbackForm.setErrors({'cdSubmitButton': true});
+  }
 }

This component should be used each time you define a new modal. This will allows us to keep all modals with the same visual aspect. Signed-off-by: Tiago Melo <tmelo@suse.com> Signed-off-by: Stephan Müller <smueller@suse.com>

You can now simply use a deletion dialog without having to import a lot of different things from ngx-bootstrap. Its easy to extend the dialog by a detail description. Signed-off-by: Tiago Melo <tmelo@suse.com> Signed-off-by: Stephan Müller <smueller@suse.com>

This change was made because a link can be placed anywhere instead of a button element. Signed-off-by: Stephan Müller <smueller@suse.com>

Due to CSS problems the link solution wasn't the best way, now it will represent only the modal content. The downside of this solution is that it put's the burden on the developer to use it the right way and import a view things to get it working. But on the upside CSS styles will work as expected. The unit test example was updated accordingly this way it should be easy to understand how it can be implemented the right way. Signed-off-by: Stephan Müller <smueller@suse.com>

Devp00l · 2018-04-24T11:59:27Z

@ricardoasmarques implemented your requested changes with unit tests

ricardoasmarques · 2018-04-24T12:12:14Z

@Devp00l this PR has a javascript error because you haven't applied all my patch, you missed this part:

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rollback-confirmation-modal/rollback-confimation-modal.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rollback-confirmation-modal/rollback-confimation-modal.component.ts
index 596c6e9427..b95d7e4704 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rollback-confirmation-modal/rollback-confimation-modal.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rollback-confirmation-modal/rollback-confimation-modal.component.ts
@@ -32,4 +32,8 @@ export class RollbackConfirmationModalComponent implements OnInit {
   submit() {
     this.onSubmit.next(this.snapName);
   }
+
+  stopLoadingSpinner() {
+    this.rollbackForm.setErrors({'cdSubmitButton': true});
+  }
 }

FYI, this is the error, when trying to rollback an RBD snapshot:

ERROR Error: Uncaught (in promise): TypeError: _this.modalRef.content.stopLoadingSpinner is not a function
TypeError: _this.modalRef.content.stopLoadingSpinner is not a function
    at eval (rbd-snapshot-list.component.ts:210)

This replaces usage of "delete-confirmation-modal" with the use of "delete-modal". Signed-off-by: Stephan Müller <smueller@suse.com>

Devp00l · 2018-04-24T12:46:08Z

@ricardoasmarques addressed your comment

ricardoasmarques

Thanks, approved!

Devp00l added DNM dashboard labels Mar 14, 2018

Devp00l requested review from tspmelo, votdev and ricardoasmarques March 14, 2018 15:55

Devp00l changed the title ~~mgr/dashboard_v2: Adds reusable deletion dialog~~ [DNM] mgr/dashboard_v2: Adds reusable deletion dialog Mar 14, 2018

LenzGr changed the title ~~[DNM] mgr/dashboard_v2: Adds reusable deletion dialog~~ [DNM] mgr/dashboard: Adds reusable deletion dialog Mar 16, 2018

Devp00l force-pushed the deletion-dialog branch from 315d078 to 6e451d1 Compare March 23, 2018 14:44

Devp00l removed the DNM label Mar 23, 2018

ricardoasmarques suggested changes Mar 23, 2018

View reviewed changes

votdev requested changes Mar 26, 2018

View reviewed changes

Devp00l force-pushed the deletion-dialog branch 2 times, most recently from 430f5d9 to 3819dbf Compare April 6, 2018 14:34

Devp00l changed the title ~~[DNM] mgr/dashboard: Adds reusable deletion dialog~~ mgr/dashboard: Adds reusable deletion dialog Apr 6, 2018

votdev reviewed Apr 10, 2018

View reviewed changes

tspmelo suggested changes Apr 10, 2018

View reviewed changes

votdev reviewed Apr 11, 2018

View reviewed changes

tspmelo previously requested changes Apr 11, 2018

View reviewed changes

votdev mentioned this pull request Apr 11, 2018

mgr/dashboard: Add RGW user and bucket management features #21351

Merged

ricardoasmarques mentioned this pull request Apr 12, 2018

mgr/dashboard: RBD management (frontend) #21385

Merged

LenzGr added the feature label Apr 13, 2018

LenzGr added this to the mimic milestone Apr 13, 2018

Devp00l force-pushed the deletion-dialog branch from 3819dbf to 8ed827d Compare April 13, 2018 11:49

Devp00l force-pushed the deletion-dialog branch 2 times, most recently from a79fc33 to 91f1112 Compare April 16, 2018 09:20

votdev requested changes Apr 16, 2018

View reviewed changes

Devp00l force-pushed the deletion-dialog branch 2 times, most recently from f4dcc08 to cb58f23 Compare April 18, 2018 15:25

Devp00l force-pushed the deletion-dialog branch from cb58f23 to 992bcff Compare April 20, 2018 14:18

votdev requested changes Apr 23, 2018

View reviewed changes

Devp00l force-pushed the deletion-dialog branch from 992bcff to 3f3f803 Compare April 23, 2018 16:57

Tiago Melo and others added 4 commits April 24, 2018 13:57

mgr/dashboard: Add modal component

f1c2f91

This component should be used each time you define a new modal. This will allows us to keep all modals with the same visual aspect. Signed-off-by: Tiago Melo <tmelo@suse.com> Signed-off-by: Stephan Müller <smueller@suse.com>

mgr/dashboard: Changes deletion button to link

f2096e9

This change was made because a link can be placed anywhere instead of a button element. Signed-off-by: Stephan Müller <smueller@suse.com>

Devp00l force-pushed the deletion-dialog branch from 3f3f803 to af4ebc6 Compare April 24, 2018 11:57

mgr/dashboard: Replaces delete-confirmation-modal

d110eba

This replaces usage of "delete-confirmation-modal" with the use of "delete-modal". Signed-off-by: Stephan Müller <smueller@suse.com>

Devp00l force-pushed the deletion-dialog branch from af4ebc6 to d110eba Compare April 24, 2018 12:45

ricardoasmarques approved these changes Apr 24, 2018

View reviewed changes

votdev approved these changes Apr 24, 2018

View reviewed changes

LenzGr merged commit adf505f into ceph:master Apr 24, 2018

ricardoasmarques mentioned this pull request Apr 26, 2018

mgr/dashboard: Fix missing $event on deletion modal #21667

Merged


		constructor(private modalService: BsModalService) {}

		openModal(template: TemplateRef<any>) {

mgr/dashboard: Adds reusable deletion dialog #20899

mgr/dashboard: Adds reusable deletion dialog #20899

Conversation

Devp00l commented Mar 14, 2018 • edited

Devp00l commented Mar 14, 2018 • edited

votdev commented Mar 23, 2018

ricardoasmarques left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

votdev commented Mar 26, 2018

Choose a reason for hiding this comment

Devp00l Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardoasmarques commented Apr 9, 2018

Devp00l commented Apr 9, 2018

ricardoasmarques commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Devp00l commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

votdev commented Apr 16, 2018

ricardoasmarques commented Apr 16, 2018

ricardoasmarques commented Apr 19, 2018

Devp00l commented Apr 20, 2018

ricardoasmarques commented Apr 23, 2018

Choose a reason for hiding this comment

Devp00l commented Apr 23, 2018

ricardoasmarques commented Apr 24, 2018

Devp00l commented Apr 24, 2018

ricardoasmarques commented Apr 24, 2018 • edited

Devp00l commented Apr 24, 2018

ricardoasmarques left a comment

Choose a reason for hiding this comment

Devp00l commented Mar 14, 2018 •

edited

Devp00l commented Mar 14, 2018 •

edited

Devp00l Apr 6, 2018 •

edited

ricardoasmarques commented Apr 24, 2018 •

edited