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

[Refactor] Added a way to display errors at a form rather than a field level #274

Open
wants to merge 1 commit into
base: 1.8
Choose a base branch
from

Conversation

pupi1985
Copy link
Contributor

It is not possible to display errors in the form the same way it is possible to display a successful message with the 'ok' form key. This should add the ability to display errors just adding the 'error'key to the form array.

Removed CSS classes: qa-form-tall-ok and qa-form-wide-ok
Added CSS classes: qa-form-tall-result-ok, qa-form-wide-result-ok, qa-form-tall-result-error, qa-form-wide-result-error

A more backwards compatible change would have been to keep the removed CSS classes and only add the new ones. However, as qa-form-tall-error and qa-form-wide-error were already used for field errors, it makes more sense to name the ones used for forms in a different way.

Similarly, duplicating the form_ok function in the base theme using a form_error function would add unnecessary code that would eventually be grouped together.

Taking this into account, making a not so backwards compatible change made sense after all.

@svivian
Copy link
Collaborator

svivian commented Aug 10, 2015

Nice idea. One other consideration would be how to activate this from filter plugins - currently filters are only passed the fields in order to set errors on them, not the form as a whole.

@svivian
Copy link
Collaborator

svivian commented Nov 14, 2015

Hi @pupi1985 just taking a closer look at this now. Do we really need the $use_form_result property? I think we can just call the new form_result method, keeping the form_ok method there for backwards-compatibility.

I realise there is the case where someone has overridden the form_ok method in their theme but I think that is minor enough that we don't need to worry too much. I don't want to add too many properties to the theme class for minor things. If you're able to change that I can merge this in if you target the PR to the 1.8 branch.

@pupi1985
Copy link
Contributor Author

Yes, $use_form_result can be removed as it was there just to allow a quick way to get the old (current) behaviour back. If it's OK just to mention the interface change in the changelog then it can be removed.

Regarding the content of the deprecated method, I think it shouldn't be removed (think about the case in which a user calls form_ok from a custom method in a custom theme or layer). I guess it is better to leave it there because if the user is manually calling form_ok then most likely has overridden (and probably, fully replaced) the method that calls form_ok (form_body) which means form_result won't be called.

Regarding filter modules... hmm... let's leave that for another day 😅

PS: I've just rebased and removed $use_form_result. Make a note to add this to the changelog, clarifying the deprecation

public function form_result($form, $columns)
{
$key = null;
if (!empty($form['ok']))

Choose a reason for hiding this comment

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

if MUST have { and }

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