Add generic confirmation modal #389

jesusreal · 2019-01-30T15:00:03Z

Description

Changes proposed in this pull request:

Implement generic confirmation modal, based on promises and reusing existing confirmation modal for unsaved changes
In angular example app, use 'projects/pr1' for Luigi client ux manager methods and 'projects/pr2' for Luigi client link manager methods
In angular sample app, add example and e2e test for luigi confirmation modal
Add documentation for luigi client
No need for html sanitation, Svelte is doing it by itself

Related issue(s)

…d-confirmation-boxes # Conflicts: # core/examples/luigi-sample-angular/e2e/support/commands.js # core/examples/luigi-sample-angular/e2e/tests/luigi-client-features.spec.js

dariadomagala-sap

Please check comments.

dariadomagala-sap · 2019-01-31T08:32:34Z

client/src/luigi-client.js

@@ -10,7 +10,9 @@ var _onContextUpdatedFns = {};
 var _onInitFns = {};
 var authData = {};
 var pathExistsPromises = {};
-
+let promises = {


Is there a need to make it an object inside an object? Why not just confirmationModalPromise object?

I created it having in mind a future refactoring to include all promises in same object. Apart from this one, we have pathExists promise, and we will soon have generic alert promise. This solution scales better in my opinion

dariadomagala-sap · 2019-01-31T08:48:52Z

core/examples/luigi-sample-angular/src/app/project/project.component.html

+          </p>
+          <p class="fd-has-font-style-italic" data-cy="luigi-confirmation-modal-result"
+            ngClass="{{(confirmationModalResult.currentKey === 'confirmed') ? 'fd-has-color-status-1' : 'fd-has-color-status-2'}}">
+            {{confirmationModalResult.info[confirmationModalResult.currentKey]}}


Wouldn't that be easier to just have Confirmation modal has been {{confirmationModalResult.currentKey}} instead of creating this confirmationModalResult .info object?

I totally agree with you, this is overcomplicated. Thanks for pointing that out. Refactored

dariadomagala-sap · 2019-01-31T08:53:50Z

docs/luigi-client-api.md

+
+#### Parameters
+
+-   `content` **[Object](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object)** Text for different elements in confirmation modal


We should add what elements the user can define.

Absolutely, I have added them

dariadomagala-sap · 2019-01-31T09:20:33Z

core/src/ConfirmationModal.html

@@ -21,15 +21,19 @@ <h1 class="fd-modal__title">{title}</h1>
 <script type="text/javascript">
  export default {
    data() {
-      //fallback data in case title or text props weren't provided
+      // fallback data in case not provided


Well, this fallback data doesn't work as it's supposed to.

So we have a confirmation modal component. We are using it currently once in App.html, in this case, we are alway providing the data to the component. Whenever you provide the data, it will never use the fallback data, even if the data you provide is undefined. I also tried with single variables instead of an object. If you give variables as input when using the component, it will never take the default values.
Default values would be taken if we use somewhere else the component and we do not give the 'content' as input. We could make it more complicated to match what you (and mostly anyone) would expect. Let´s discuss the best solution offline.

Added mechanism to set default values if not provided. If values are provided, even with a falsy value like empty string or undefined, they will be taken

bszwarc

Just minor things

client/src/luigi-client.js

Co-Authored-By: jesusreal <8238188+jesusreal@users.noreply.github.com>

bszwarc

small fix

bszwarc · 2019-02-01T06:09:01Z

client/src/luigi-client.js

+       * @param {string} content.body the content of the modal body
+       * @param {string} content.buttonConfirm the label for the modal confirm button
+       * @param {string} content.buttonDismiss the label for the modal dismiss button
+       * @returns {promise} A promise which is resolved when accepting confirmation modal and rejected when dismissing it.


Suggested change

* @returns {promise} A promise which is resolved when accepting confirmation modal and rejected when dismissing it.

* @returns a {promise} which is resolved when accepting the confirmation modal and rejected when dismissing it.

This is not correct regarding documentation. First comes the type of the returned value and the the additional info. I will just add the "the"

Ok :) I was just looking at the entire document, and under has Back I've found:
Returns boolean indicating if there is a preserved view you can return to. that's why I wanted to adjust the other part of the doc. Should we change this one as well, or is it a different case?

Also let's then write a promise - lower case, to make it consistent :)

The piece of documentation you mentioned is:

* @returns {boolean} indicating if there is a preserved view you can return to.

I will replace the line we are talking about with:

* @returns {promise} which is resolved when accepting the confirmation modal and rejected when dismissing it.

This makes it consistent imho. Is that ok for you?

dariadomagala-sap

Splitting uxManager and linkManager methods to different projects (probably) caused problems.

open the app in /overview
go to projects using top nav
go to pr1 using left nav
click add backdrop- nothing happens
click confirmation modal - modal is opened, but there is no message if it has been confirmed / rejected

Works properly if I open the app in /projects/pr1.

Also, could we move this confirmation modal to the middle of the window?

dariadomagala-sap

We should wrap words inside and prevent those kind of situations. In console we could have long errors (k8s resources can have I think 63 letters long names) and we should display them properly.

dariadomagala-sap · 2019-02-04T13:00:18Z

core/src/ConfirmationModal.html

-    box-sizing: border-box;
-    width: $width;
-    max-width: 90vw;
-    top: calc(#{$topNavHeight} + 1rem);
    left: calc(50% + #{$leftNavWidth}/ 2 - #{$width}/ 2);


This should be removed as well if we want to have it in the middle of the screen.

I will fix the 1st one. For the 2nd one, I will clarify it with Franz

jesusreal added 9 commits January 29, 2019 12:12

First working implementation of confirmation modal

fe3eb92

Merge remote-tracking branch 'upstream/master' into generic-alerts-an…

240fcd9

…d-confirmation-boxes # Conflicts: # core/examples/luigi-sample-angular/e2e/support/commands.js # core/examples/luigi-sample-angular/e2e/tests/luigi-client-features.spec.js

add e2e test and replace 'accept' with 'confirm' for modal button

ac5e8b4

Fix couple of e2e tests and remove some e2e test code not needed

0998519

refactor around promises for confirmation modal

7f5476e

text changes

e9eca1c

Fix pathExists functionality broken in last refactoring commit

0c37cf7

Uncomment e2e tests

748583b

Proper data on confirmationModal state object

9887bbd

jesusreal added enhancement New feature or request area/luigi labels Jan 30, 2019

jesusreal requested review from antiheld, bszwarc, dariadomagala-sap, hardl, JohannesDoberer, kazydek, klaudiagrz, kwiatekus, marynaKhromova, maxmarkus, parostatkiem-zz, pekura, tomekpapiernik and y-kkamil as code owners January 30, 2019 15:00

jesusreal mentioned this pull request Jan 30, 2019

Generic alerts and confirmation boxes #72

Closed

dariadomagala-sap self-assigned this Jan 31, 2019

dariadomagala-sap suggested changes Jan 31, 2019

View reviewed changes

JohannesDoberer self-assigned this Jan 31, 2019

improve code for confirmation modal result being displayed

eabb55d

jesusreal added 2 commits January 31, 2019 13:37

Improve API and docu for confirmation modal content

88da0df

Update luigi client readme file

40c9f82

bszwarc reviewed Jan 31, 2019

View reviewed changes

client/src/luigi-client.js Outdated Show resolved Hide resolved

client/src/luigi-client.js Outdated Show resolved Hide resolved

client/src/luigi-client.js Outdated Show resolved Hide resolved

client/src/luigi-client.js Outdated Show resolved Hide resolved

bszwarc and others added 5 commits January 31, 2019 15:31

Update client/src/luigi-client.js

1ea9034

Co-Authored-By: jesusreal <8238188+jesusreal@users.noreply.github.com>

Update client/src/luigi-client.js

761c132

Co-Authored-By: jesusreal <8238188+jesusreal@users.noreply.github.com>

Update client/src/luigi-client.js

ecd0752

Co-Authored-By: jesusreal <8238188+jesusreal@users.noreply.github.com>

Update client/src/luigi-client.js

080368c

Co-Authored-By: jesusreal <8238188+jesusreal@users.noreply.github.com>

Update luigi client readme file after feedback from technical writer

93a4931

bszwarc reviewed Feb 1, 2019

View reviewed changes

Very small improvement in docu

ed3fc89

dariadomagala-sap suggested changes Feb 4, 2019

View reviewed changes

jesusreal added 4 commits February 4, 2019 10:49

Proper mechanism to use default values for modal

5276171

small docu improvement

ae183e8

Fix issue with ngIf preventing project detail view from working properly

d7deb89

Center confirmation modal vertically by removing unnecessary styles

9f6f5cb

bszwarc approved these changes Feb 4, 2019

View reviewed changes

dariadomagala-sap reviewed Feb 4, 2019

View reviewed changes

jesusreal added 2 commits February 4, 2019 14:40

Break long words in confirmation modal header and body

cd6e607

Confirmation modal now is viewport centered, not iframe centered

89e4701

dariadomagala-sap approved these changes Feb 4, 2019

View reviewed changes

JohannesDoberer approved these changes Feb 4, 2019

View reviewed changes

jesusreal merged commit cab97a4 into SAP:master Feb 5, 2019

jesusreal deleted the generic-alerts-and-confirmation-boxes branch March 13, 2019 10:29

misappi unassigned dariadomagala-sap May 14, 2019

stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021

Add generic confirmation modal (SAP#389)

c811050

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add generic confirmation modal #389

Add generic confirmation modal #389

jesusreal commented Jan 30, 2019

dariadomagala-sap left a comment

dariadomagala-sap Jan 31, 2019

jesusreal Jan 31, 2019

dariadomagala-sap Jan 31, 2019

jesusreal Jan 31, 2019

dariadomagala-sap Jan 31, 2019

jesusreal Jan 31, 2019

dariadomagala-sap Jan 31, 2019

jesusreal Jan 31, 2019

jesusreal Feb 4, 2019

bszwarc left a comment

bszwarc left a comment

bszwarc Feb 1, 2019

jesusreal Feb 1, 2019

bszwarc Feb 1, 2019

jesusreal Feb 4, 2019

dariadomagala-sap left a comment

dariadomagala-sap left a comment

dariadomagala-sap Feb 4, 2019

jesusreal Feb 4, 2019


		#### Parameters

		- `content` [Object](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object) Text for different elements in confirmation modal

	* @returns {promise} A promise which is resolved when accepting confirmation modal and rejected when dismissing it.
	* @returns a {promise} which is resolved when accepting the confirmation modal and rejected when dismissing it.

Add generic confirmation modal #389

Add generic confirmation modal #389

Conversation

jesusreal commented Jan 30, 2019

dariadomagala-sap 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bszwarc left a comment

Choose a reason for hiding this comment

bszwarc 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

dariadomagala-sap left a comment

Choose a reason for hiding this comment

dariadomagala-sap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment