Skip to content
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

Implement nice validation error display #191

Merged
merged 3 commits into from
Jul 9, 2017

Conversation

romaninsh
Copy link
Member

Models now throw "ValidationError" when something is wrong with the data. That's not an applicaiton error, if it's left un-cached then the error should be properly presented to the user.

The form already does that by highlighting appropriate field with the error message, but the standard callback handlers based around jsCallback would show an ugly message.

This PR will display validation errors nicely, if developer didn't bother to take care of them.

Demo now have a [failure] button:

screen shot 2017-07-04 at 12 29 20

Click it to get this prompt:

screen shot 2017-07-04 at 12 29 26

The responsible code is:

$b = $layout->add(new Button('failure'));
$b->on('click', function ($b) {
    throw new \atk4\data\ValidationException(['Everything is bad']);
});

@romaninsh romaninsh requested a review from DarkSide666 July 4, 2017 11:32
@@ -53,44 +53,60 @@ public function set($callback, $args = [])
}

return parent::set(function () use ($callback) {
$chain = new jQuery(new jsExpression('this'));
Copy link
Member Author

@romaninsh romaninsh Jul 4, 2017

Choose a reason for hiding this comment

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

there are no changes here - only alignment.

}, $actions));

$this->app->terminate(json_encode(['success'=>true, 'message'=>'Success', 'eval'=>$ajaxec]));
} catch (\atk4\data\ValidationException $e) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is new code added. Everything above is just re-aligned.

Copy link
Member

Choose a reason for hiding this comment

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

  1. As Alain said in his comment - not sure why we need message and eval=alert both at the same time.

  2. I still don't know how to run UI JS on my local repo. JS methods simply return json and nothing catches it.
    See video: https://www.useloom.com/share/c81f35682c5648ea8f1885fd5736c4dc

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought - for compatibility, but didn't seem to work :) I'll drop the eval.

Copy link
Member

Choose a reason for hiding this comment

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

You can remove more code above which fills now unused $ajaxec variable etc.

]);
$app = new \atk4\ui\App();

$app->title = 'Agile UI - Demo Suite';
Copy link
Member Author

@romaninsh romaninsh Jul 4, 2017

Choose a reason for hiding this comment

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

The version is now displayed in the footer, why duplicate.

@romaninsh romaninsh requested a review from ibelar July 4, 2017 12:55
Copy link
Contributor

@ibelar ibelar left a comment

Choose a reason for hiding this comment

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

You can probably just do;
catch (\atk4\data\ValidationException $e) {
// Validation exceptions will be presented to user in a friendly way
$this->app->terminate(json_encode(['success'=>false, 'message'=>$e->getHTML()]));
}

I do not think you need to render the $action. Plus sending the exception directly will have the full stack as well display to user.

@ibelar
Copy link
Contributor

ibelar commented Jul 5, 2017 via email

@romaninsh romaninsh merged commit 23819e9 into develop Jul 9, 2017
@romaninsh romaninsh deleted the feature/validation-dimmer branch July 9, 2017 10:05
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.

3 participants