Add optional promise-aware behavior to frost-modal-form #125

gknoy · 2018-01-08T22:55:52Z

This project uses semver, please check the scope of this pr:

#none# - documentation fixes and/or test additions
#patch# - backwards-compatible bug fix
#minor# - adding functionality in a backwards-compatible manner
#major# - incompatible API change

Overview

This PR adds optional flags and behavior to the frost-modal-form component which allows it to behave in a convenient way when onConfirm returns a Promise.

Automatically close once `onConfirm` resolves

The closeOnConfirm behavior has been upgraded to be Promise aware. If onConfirm returns a Promise, the modal will not call onClose until the Promise resolves.

{{frost-modal-form
  closeOnConfirm=true
  ....
  onConfirm=(action 'somethingThatReturnsAPromise')
}}

Previously, closeOnConfirm would ignore the return value of onConfirm, and invoke onClose immediately. Consumers who wanted promise aware behavior have needed to disable closeOnConfirm and handle it themselves (which is still valid).

Disables the confirm button until `onConfirm` resolves

When onConfirm returns a Promise, we now prevent spamming of the Confirm button by disabling it until the promise resolves. setting the closeOnConfirm to false, and disableConfirmUntilOnConfirmResolves to true.

The text to be shown while disabled can be changed by overriding the disabledConfirmText property.

{{frost-modal-form
  closeOnConfirm=true
  disableConfirmUntilOnConfirmResolves=true
  // optional override of button props:
  confirm=(object
    disabledText='Waiting'
    text='Confirm'
  )
  ....
}}

If you do not want the Confirm button to be disabled while resolving the onConfirm return value, you can set disableConfirmUntilOnConfirmResolves=false:

{{frost-modal-form
  closeOnConfirm=true
  disableConfirmUntilOnConfirmResolves=false
  ....
}}

Screenshot

Demo updates

The modal form demo has been rewritten, so that there are samples of basic usage, promise-aware usage, and a demo of nearly all the bells and whistles (the original form demo route). Each of these demos integrate the Bunsen form's validation state, as one nearly always wants the Confirm button to be disabled until the form is valid.

Changelog

Upgraded closeOnConfirm to be promise-aware. If onConfirm returns a Promise, the modal will call onClose once that Promise resolves. (Errors will leave the modal open.)
Added optional disableConfirmUntilOnConfirmResolves flag, and the ability to specify different text for a disabled confirm button. This behavior defaults to true, and is only active when handling an onConfirm that returns a Promise.
Upgraded demo pages for frost-modal-form so that they show more realistic form modal usage
Added support for title tooltips on modal buttons
Added chai-jquery, sinon-chai devDependencies
Updated to version ^8.1.0 of ember-test-utils
Added eslint configuration options to allow existing test names
Fixed some linting per updated linting rules
Fixed demo notifications so that they show above the blur layer

### Overview This adds optional flags and behavior to the `frost-modal-form` component which allows it to behave in a convenient way when `onConfirm` returns a promise. #### Disabling the confirm button until `onConfirm` resolves When `onConfirm` triggers an action that is either expensive or not idempotent, we now can prevent spamming of the Confirm button by setting the `closeOnConfirm` to false, and `disableConfirmUntilOnConfirmResolves` to true. The text to be shown while disabled can be changed by overriding the `disabledConfirm` property. ``` {{frost-modal-form closeOnConfirm=false disableConfirmUntilOnConfirmResolves=true // optional override of button props: disabledConfirm=(object disabled=true isVisible= // (true) text= // ('Confirm') ) .... }} ``` #### Automatically close once `onConfirm` resolves ``` {{frost-modal-form closeOnConfirm=false closeAfterOnConfirmResolves=true .... }} ``` #### Demo updates The modal form demo has been expanded with an extra section (`/#/form-async`) demonstrating the new behavior. # Changelog - Add optional `closeAfterOnConfirmResolves` flag - Add optional `disableConfirmUntilOnConfirmResolves` flag - Allow forcing disabled confirm button state until resolve - Show demo notifications above the blur layer - Add support for title tooltips on modal buttons - Update to ember-test-utils@^8.1.0 - Add eslint rules to allow existing test names - Fix linting per updated linting rules - Add chai-jquery, sinon-chai devDependencies

coveralls · 2018-01-08T23:05:58Z

Changes Unknown when pulling 603af8f on gknoy:gknoy/add-promise-aware-confirmation into ** on ciena-frost:master**.

cstolli · 2018-01-09T17:04:39Z

👍

notmessenger · 2018-01-09T16:57:12Z

addon/components/dialogs/form.js

-  params () {
+  @computed('forceDisabledConfirm')
+  params (forceDisabledConfirm) {
+    // We look these up here, rather than using arguments to computed(), so that we avoid double-computed errors


All you have done is prevent a log message from appearing - you have not stopped double renders from occurring or the work associated with the renders. The objective should not be to avoid the error message - it should be to avoid the scenario that causes the error to occur.

notmessenger · 2018-01-09T17:02:28Z

tests/dummy/app/pods/application/template.hbs

-    {{notification-message notification=notification}}
-  {{/each}}
-</div>
+{{notification-container zindex=9999}}


Doesn't zindex need a -?

It does in plain css

This isn't setting the style or CSS property, though, it's an argument to the Ember notification widget. The widget now sets its z-index explicitly with a style=, and the way to override that is with a component property named zindex.

http://stonecircle.github.io/ember-cli-notifications/

https://github.com/stonecircle/ember-cli-notifications/blob/086951c588867c165f8776abb2fde769d470b31c/addon/components/notification-container.js#L14

gkchestertron · 2018-01-09T17:07:38Z

This approach works, but I think adds unnecessarily to the interface. This behavior is probably what a developer will want more than 90% of the time, and there is currently no behavior when you hand a promise to the handler, so why don't we just make the disable-and-wait behavior default when given a promise? You can still do weird things by setting onConfirm to false and writing your own.

notmessenger · 2018-01-09T17:08:04Z

I would like to discuss the approach that has been chosen to add the desired functionality. I'd much rather prefer to see a pending property be introduced that when false allows the confirm button to be enabled and when true for it to be disabled. This allows for the management of the state of the modal to be managed externally - which is essentially what your current approach does but has a lot of code needed to do so - without introducing additional properties that a user has to manage in certain combinations to get things working correctly, while increasing the surface area of the API. The closing of the modal will need to be addressed but can discussed as well.

gkchestertron

I think this should be default behavior.

- Computes `confirmButtonProps` based on confirm + forceDisabledConfirm (currently only used by the form modal) - Updates `frost-modal-form` demos to show basic usage, promise-aware behavior, and extra options (the old demo, which had almost all the bells and whistles). - Tests of confirm button stuff use the test utils to check button state ;)

coveralls · 2018-01-09T23:54:36Z

Changes Unknown when pulling 83d2f66 on gknoy:gknoy/add-promise-aware-confirmation into ** on ciena-frost:master**.

gknoy · 2018-01-10T00:04:57Z

I've updated the PR description to reflect the changes to the PR:

default behavior of closeOnConfirm is upgraded
demo app has nicer demos of the modal form

I think this still counts as a minor change, but it's conceivable that the new promise-aware behavior might break some expectations (specifically the disabling of the Confirm button). I suspect that most people that care about promise-aware interactions with a modal are already handling that themselves, though. Let me know if I need to edit the PR to be a Major -- or you can edit it to be so, I expect. :)

gkchestertron · 2018-01-10T00:42:09Z

addon/components/frost-modal-binding.js

+      text: PropTypes.string,
+      title: PropTypes.string
+    }),
+    disabledConfirmText: PropTypes.string,


This should probably go into the confirm namespace.

gkchestertron · 2018-01-10T00:43:00Z

addon/components/frost-modal-binding.js

+      title: PropTypes.string
+    }),
+    disabledConfirmText: PropTypes.string,
+    disableConfirmUntilOnConfirmResolves: PropTypes.bool,


Is this flag necessary? Can't they just not return a promise and figure out how to handle it in their own way?

Also, if we need this - it should probably also go into the confirm namespace.

~~I like your suggestion of putting the this and the alternate text in the confirm property.~~

I still like your suggestion, but since we want this flag to default to true, I'd like consumers to be able to not need to specify it every time they override anything in config. Per our discussion, I'll leave this as it is but move the text setting to the config namespace. :)

gkchestertron · 2018-01-10T00:44:29Z

addon/components/frost-modal-binding.js

@@ -31,6 +52,9 @@ const FrostModalBinding = Component.extend(PropTypesMixin, {
    noBlur: PropTypes.bool,
    targetOutlet: PropTypes.string,

+    // State
+    forceDisabledConfirm: PropTypes.bool, // only used when we are currently overriding the confirm button state


I don't understand why this is part of the interface. It's late though... may just be my brain.

forceDisabledConfirm was not intended for the public interface, but because we want to initialize it in getDefaultProps(), our convention is to declare the state variables in PropTypes as well. That's not really necessary, just a habit from the other apps I work with.

That said, it is also possible to fiddle that field ourself when we are opting to handle all of the onConfirm/onClose interactions ourself (by setting closeOnConfirm=false). I'm not sure if that means we should declare it in the props interfaces, or leave that as a "private" flag which users can use at their own risk.

coveralls · 2018-01-10T19:06:13Z

Changes Unknown when pulling 81410c0 on gknoy:gknoy/add-promise-aware-confirmation into ** on ciena-frost:master**.

notmessenger · 2018-01-10T19:23:01Z

.eslintrc.js

@@ -2,5 +2,13 @@ module.exports = {
  extends: 'frost-standard',
  globals: {
    capture: false
+  },
+  rules: {


Why were these added? Did you encounter any issues when you didn't have them? (Wouldn't have expected you too since you've upgraded ember-test-utils to ^8.1.0)

Yes.

Not sure on prroptypes, but complaining about Wrong Ones seems good

valid-test-descriptions will fail if it(...) descriptions don't start with 'should ...'. This makes nearly every test in the repo fail linting. We can either grandfather them in temporarily, or permanently -- I didn't see much to gain from rewriting all of the test descriptions.

the no-multi-spaces one means you can put // two spaces in front of a comment, which is nice but not necessary.

The rules introduced by upgrading ember-test-utils to ^8.1.0 are the latest rules we are following as an organization. As such your 2nd and 3rd bullet points need to be addressed, which means that the appropriate changes need to be made and not add additional repo-specific rules. For the first bullet point if this is not already being provided by ember-test-utils@^8.1.0 then an issue needs to be opened against it, as what is good for one repo is good for all repos.

The tests for almost everything in this repo are written in a non-idiomatic style; is it OK to defer fixing those to a follow-up PR? I'd like to have that be a separate PR both for legibility and because I had hoped to be able to use this feature this week. ;-)

I'm not sure why you upgraded ember-test-utils as part of this PR but since you did then we should do the work that is involved with its upgrade as well.

Besides, it's only Wednesday and the required changes shouldn't take more than 15-20 minutes tops ;-)

notmessenger · 2018-01-10T19:24:21Z

addon/components/frost-modal-binding.js

+ * @returns {Boolean} whether thing is a thenable
+ */
+function isThenable (thing) {
+  return (thing !== undefined) && (typeof thing.then === 'function')


There should be a check added in between these two for the existence of thing.then

If thing is not defined, then thing.then will simply resolve as undefined, and typeof that will still work:

> thing = {wat: 13} {wat: 13} > typeof(thing.then) "undefined"

Oh yeah, right. Thanks :-)

notmessenger · 2018-01-10T19:27:45Z

addon/components/frost-modal-binding.js

+  @computed('confirm', 'forceDisabledConfirm', '_defaultConfirm')
+  /**
+   * Computed our Confirm button's props so that we can alter text/disabled in concord with onConfirm promises
+   * TODO: Update confirm/error/info/warn to use this as well: they currently use this.confirm directly


Add an issue about this if you haven't already addressed this if. If you have please remove the comment.

I added #127 to describe this.

Is it time-intensive to just address these now?

My initial impression when I first encountered the linting errors was that it was, but let me look further.

Javascript: 52 errors, 0 warnings

It's updating ~50 tests. Most of them should be text changes, so it's probably not so bad. I'll just update the test names, though, and not the style of expectations.

notmessenger · 2018-01-10T19:38:11Z

addon/components/frost-modal-binding.js

+            })
+          }
+
+          confirmed = confirmed.then(() => {


Shouldn't this be in an else?

No. The else for the prior if is to do nothing regarding the disabled state (and would therefore be empty).

This is merely the action we want to happen within the if closeOnConfirm ... branch, namely call onClose in a promise-aware way.

confirmed = confirmed.then(() => { this.onClose() })

is always going to run with this block of code. The if statement before it

if (this.get('disableConfirmUntilOnConfirmResolves')) { this.set('forceDisabledConfirm', true) confirmed = confirmed.then(() => { this.set('forceDisabledConfirm', false) }) }

may optionally set confirmed to something as well.

So if the first if statement is executed it doesn't matter what confirmed was set to because the code setting confirmed after the if statement is always going to run.

Or am I missing something?

Because they're in the same block, you can re-assign promises. It's re-using the confirmed variable.

I previously did

let thenable = confirmed if (....) { thenable = confirmed.then( /* fix button */ ) } thenable = confirmed.then( /* invoke onClose */ ) return thenable

but then realized that thenable was completely redundant, because one can do the same reassignment with confirmed anyway.

The end result is returning a promise that is basically

confirmed .then(() => {...}) // invoke onClose .finally(() => { ... }) // fix button state

(or should be once I fix the bug I commented about in a sister comment :))

notmessenger · 2018-01-10T19:38:45Z

addon/components/frost-modal-binding.js

      if (onConfirm) {
-        onConfirm()
+        confirmed = onConfirm()


What happens if this fails? Line 165 or 170 isn't going to run.

If the onConfirm promise fails, the dialog will not close, because of the way .then chaining works. This is good (IMO) because it lets us keep the dialog open while an error notification (shown by onConfirm) is displayed, allowing the user to fix their input.

For example, one might try to Create a New User, and get an error back form the API that the email address is already in use, or some other server-knowledge-only error

Good catch though about the state of the disabled button not working right, though. I don't want onClose to be invoked, but I still want the confirm button to be re-enabled even if the promise is rejected. Can I do

confirmed .finally(() => { ... }) // fix button state .then(() => {...}) // invoke onClose

or do I need to move this block so that the final action (re-enabling confirmation) is done after the .onClose() invocation so that I end up with

confirmed .then(() => {...}) // invoke onClose .finally(() => { ... }) // fix button state

?

notmessenger · 2018-01-10T20:19:24Z

addon/components/frost-modal-binding.js

+    /**
+     * Handle confirmation in a promise-aware way.
+     * If onConfirm returns a promise, closeOnConfirm will wait for it to resolve successfully before closing the modal.
+     * @returns {Object} the return value of onConfirm.


If this.get('closeOnConfirm') !== true this is not what gets returned

Good point. I think since there's no way to normally get the return value directly (it's just bound to the click handler), I had overlooked that. Thanks for pointing that out.

- frost-standard will already error when prop types are wrong, so we don't need to specify that - fix test descriptions now, rather than later

coveralls · 2018-01-10T20:29:47Z

Changes Unknown when pulling 8e39a9d on gknoy:gknoy/add-promise-aware-confirmation into ** on ciena-frost:master**.

…ehavior

gkchestertron · 2018-01-11T21:19:22Z

addon/components/frost-modal-binding.js

+    }
+
+    confirmed = confirmed.catch((err) => {
+      deps.Logger.log(err)


Do we not want it in the log as an error?

We do, thanks for catching that! I don't think I have ever used Logger.error before, so I forgot it existed.

coveralls · 2018-01-11T21:25:58Z

Changes Unknown when pulling ef040dd on gknoy:gknoy/add-promise-aware-confirmation into ** on ciena-frost:master**.

coveralls · 2018-01-11T21:35:11Z

Changes Unknown when pulling 5f196bc on gknoy:gknoy/add-promise-aware-confirmation into ** on ciena-frost:master**.

coveralls · 2018-01-11T21:38:51Z

Changes Unknown when pulling 5f196bc on gknoy:gknoy/add-promise-aware-confirmation into ** on ciena-frost:master**.

notmessenger reviewed Jan 9, 2018

View reviewed changes

gkchestertron suggested changes Jan 9, 2018

View reviewed changes

gkchestertron reviewed Jan 10, 2018

View reviewed changes

Move disabledConfirmText to confirm.disabledText

81410c0

notmessenger reviewed Jan 10, 2018

View reviewed changes

notmessenger mentioned this pull request Jan 10, 2018

Update confirm/error/info/warn dialogs to have promise-aware confirm behavior #127

Open

notmessenger reviewed Jan 10, 2018

View reviewed changes

Fix test descriptions, don't override frost-standard rules

8e39a9d

- frost-standard will already error when prop types are wrong, so we don't need to specify that - fix test descriptions now, rather than later

Gabriel Knoy added 4 commits January 10, 2018 15:13

Disentangle closeOnConfirm and disableConfirmUntilOnConfirmResolves b…

9b61e1b

…ehavior

Catch errors, allow rejectable demo promise

acbe312

Fix integration/frost-modal-form-test

f6eb5ec

Simplify _onConfirm

fd64144

gkchestertron reviewed Jan 11, 2018

View reviewed changes

Gabriel Knoy added 2 commits January 11, 2018 13:21

fix checks of closeOnConfirm

ef040dd

Use Logger.error to log promise rejections

5f196bc

gkchestertron approved these changes Jan 11, 2018

View reviewed changes

gkchestertron merged commit 97600e7 into ciena-frost:master Jan 12, 2018

Add optional promise-aware behavior to frost-modal-form #125

Add optional promise-aware behavior to frost-modal-form #125

Conversation

gknoy commented Jan 8, 2018 • edited Loading

This project uses semver, please check the scope of this pr:

Overview

Automatically close once onConfirm resolves

Disables the confirm button until onConfirm resolves

Screenshot

Demo updates

Changelog

coveralls commented Jan 8, 2018

cstolli commented Jan 9, 2018 • edited by dafortin Loading

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

gkchestertron commented Jan 9, 2018

notmessenger commented Jan 9, 2018 • edited Loading

gkchestertron left a comment

Choose a reason for hiding this comment

coveralls commented Jan 9, 2018

gknoy commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gknoy Jan 10, 2018 • edited Loading

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

coveralls commented Jan 10, 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

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

gknoy Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 11, 2018

coveralls commented Jan 11, 2018

coveralls commented Jan 11, 2018

gknoy commented Jan 8, 2018 •

edited

Loading

Automatically close once `onConfirm` resolves

Disables the confirm button until `onConfirm` resolves

cstolli commented Jan 9, 2018 •

edited by dafortin

Loading

notmessenger commented Jan 9, 2018 •

edited

Loading

gknoy Jan 10, 2018 •

edited

Loading

gknoy Jan 10, 2018 •

edited

Loading