Skip to content

pfModalOverlay: allow async validations while keeping modal open, demo maxlength validation #756

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

Merged

Conversation

dtaylor113
Copy link
Member

@dtaylor113 dtaylor113 commented Aug 9, 2018

Description

Fixes #755

Added the ability to return false for actionBtn function(s) to keep modal open. Allows app dev to click 'Save', show spinner & perform async validation, and if invalid display error msg, or if valid, programmatically close the modal:
Also demonstrated how to perform ng-maxlength validations.
modal_overlay

Also fixes #757 (Broken Semantic Release)

PR Checklist

  • Unit tests are included
  • Screenshots are attached (if there are visual changes in the UI)
  • A Designer (@beanh66) is assigned as a reviewer (if there are visual changes in the UI)
  • A CSS rep (@cshinn) is assigned as a reviewer (if there are visual changes in the UI)

@dtaylor113 dtaylor113 force-pushed the pfmodaloverlay-validations branch from d06e923 to 27cf462 Compare August 9, 2018 17:23
@beanh66 beanh66 self-requested a review August 10, 2018 13:03
Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtaylor113 This is working great! Wondering if we can adjust the visuals to match the PF "Errors and Validation" pattern.

@beanh66
Copy link
Member

beanh66 commented Aug 10, 2018

Following the Errors and Validation pattern would include an inline notification at the top with the specific field-level errors listed out by each field. Below is an example of what I mean:
errors

@dtaylor113
Copy link
Member Author

Hi @beanh66, ok, sounds good. The example here actually can have several error messages, how would that be displayed in the modal?

@beanh66
Copy link
Member

beanh66 commented Aug 10, 2018

The summary is just in the inline notification so I think that could stay the same...input is not valid, fix the errors, etc. Then the additional errors would be highlighted below. Does that make sense?

Something like this:
errors-2

@dtaylor113
Copy link
Member Author

@beanh66, ok, thanks, that makes sense. I'll work on it some more. I may create another Issue/PR for this additional work because it's not what this PR was directly addressing. -thanks

@dtaylor113
Copy link
Member Author

dtaylor113 commented Aug 10, 2018

I may create another Issue/PR for this additional work because it's not what this PR was directly addressing

...for the inline notification at the top that is, the field level help shouldn't be an issue for this PR. I need to figure out if the inline notification at the top should be baked into the component, or done via the app dev's supplied form.

@dtaylor113 dtaylor113 force-pushed the pfmodaloverlay-validations branch from 27cf462 to 8a26ac1 Compare August 13, 2018 17:09
@dtaylor113
Copy link
Member Author

Hi @beanh66, updated the screenshot/video in this PR's description. PTAL -thanks!

beanh66
beanh66 previously approved these changes Aug 13, 2018
Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dtaylor113 LGTM!

@dtaylor113 dtaylor113 force-pushed the pfmodaloverlay-validations branch 10 times, most recently from 3dde992 to 1ddd033 Compare August 13, 2018 19:34
@dtaylor113
Copy link
Member Author

@jeff-phillips-18, hoping you could take a look -thanks

@@ -103,9 +109,12 @@ angular.module('patternfly.modals').component('pfModalOverlayContent', {

ctrl.ok = function (label, actionFn) {
if (typeof actionFn === "function") {
actionFn();
if (actionFn() !== false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only concern would be an existing application returning false but not expecting the modal to remain open. Unlikely unless it is just happenstance. I think it's OK to assume that unlikelyhood is great enough to proceed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tend to agree that scenario is unlikely. If an app was returning false, I'm not sure why or what controller would be processing a true or false? The component executes the button actionFns and it wasn't expecting any return values.

@dtaylor113 dtaylor113 force-pushed the pfmodaloverlay-validations branch 4 times, most recently from 5cf8851 to 4cf62e6 Compare August 14, 2018 14:26
@dtaylor113 dtaylor113 force-pushed the pfmodaloverlay-validations branch from 4cf62e6 to 80c72b9 Compare August 14, 2018 14:43
@jeff-phillips-18 jeff-phillips-18 merged commit 09f41ab into patternfly:master Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken Semantic Release pfModalOverlay: no way to close modal for custom validations
3 participants