While working on converting the handling of errors from $context to a class, I added a small improvement I was planning to do: a template to output errors in a consistent way.
You can see it here:
https://github.com/emanuele45/Dialogo/blob/error_class/Themes/default/index.template.php#L600
At the moment is used only in the branch error_class, but since it seems nice to have, it would be good if we introduce a new error to have a consistent markup and use consistent elements.
In particular:
- $context['error_type'] => may be absent (in that case the infobox class is used), if present if the value is 'serious' the class errorbox is used, otherwise noticebox;
- $context['error_title'] => if present it most contain a "title" like the on when you get an error while posting
- $context[$error_id] => contains an array of errors in the form $key => $error_text. $key becomes part of the ID of the <li> tag, while the text is...obviously the error message :P
Of course the current template is just a mix of things, don't know if it has good markup or not (it's basically a mix of the two kinds of templates I found) of it lacks any functionality we need or we'd like to have.
The important thing I would like to keep is the id for each <li> (or other tag in case something else is more suitable) element so that it should be easier to come up with something like the current "report to moderator" page (i.e. have the errors appear and disappear while typing using javascript).
If I missed something let me know.
Lovely rrefactoring, I must say.
I saw that! And it worrks like a charm! Nice interraction ;)
/me nods approvingly, or purrs, hard to say at times.
The other ~100 possible errors, to be handled by error_context O:-)
/me runs.
(don't say it.)
/me is working on them...one by one...
I just committed the verification code and RtM.
In the meantime:
// For compatibility reasons, if this is the index template without new functions, include compatible stuff.
if (substr($template_name, 0, 5) == 'index' && !function_exists('template_button_strip'))
loadTemplate('Compat');
...can I? O:-)
I rremember seeing this and I was wonderring if it's needed at all. Methinks it isn't. :D We don't converrts old templates and stuff.
I'm looking now at the attachments errors...
What would you think about having a separated error box for those?
https://github.com/elkarte/Elkarte/blob/master/Themes/default/Errors.template.php#L216
I don't know. Why would they be separate? Disclaimer: me is losing track of variations and subvariations.
Re markup, one thing I often wonder about is when div wrappers around block level elements are desirable. Ul and dl are block level, and have basically the same properties as a div. That means that instead of this:
echo '
<div class="buttonlist', !empty($direction) ? ' float' . $direction : '', '"', (empty($buttons) ? ' style="display: none;"' : ''), (!empty($strip_options['id']) ? ' id="' . $strip_options['id'] . '"': ''), '>
<ul>',
It would be perfectly possible to use this:
echo '
<ul class="buttonlist', !empty($direction) ? ' float' . $direction : '', '"', (empty($buttons) ? ' style="display: none;"' : ''), (!empty($strip_options['id']) ? ' id="' . $strip_options['id'] . '"': ''), '>, '
Just as one example. :) To my mind, 2.0.x had a serious case of divitis in a lot of places.
@Ant: in that specific case, just because the <div> is always outputted (even if there is no error) and (in theory, in future) the errors could be inserted by javascript too, so the <div> acts like a placeholder to tell js where to put the error. Nothing more.
/me forgot about the Errors.template...
The reason why I'm proposing another box for the attachments is because they have a sort of "title" too different from the normal post errors.
So either we remove that "title", or we put them into another error box (for example below the normal "post" errors).
Ok, fair enough then. :) For the errors stuff anyway. Might still be inclined to remove it from buttonlist.
Oh and I kinda like this: "or we put them into another error box (for example below the normal "post" errors)."
/me thinks it works just fine either way, as long as I can still read the result. O:-)
/me is experimenting with javascript... lol
https://github.com/emanuele45/Dialogo/commit/98dcdc1983e80b5823f5d4bf90a0a198cac7501d
Probably the first wrong thing is that I put the code in theme.js, but since there is a bit of markup in it I thought Ant would be happy like that.
Couple of minor points.
You have ul.error and li.error in different places. I'd be inclined to change that for lower chance of confusion. Also makes for more efficient css, because with css just using class name is better than using tag name and class name.
Also here: this.error_box.append("<ul id='" + this.opt.error_box_id + "_list'></ul>");
I'd give that ul a class. In fact, if assigning classes and you don't want to assign one to both the ul and the li, in general I'd put it on the ul.
Apart from that, looks fine. Markup in js isn't necessarily a problem. Presentation in js is more likely to be a problem.
Yep, I think I was thinking to remove one of the two classes, but I forgot... lol
Before I spend some other time for nothing, better if I ask. :-[
https://github.com/emanuele45/Dialogo/commit/3e695df6a2059984ed5cb07f0c3ea062098bcd85
(I'm pretty sure it is still broken :P)
But the general question is: does it make sense?
Attachments are bad beasts, and I didn't find any other "nice" way to do that... of course I didn't try so hard to find any :P
If it makes sense, the next step would be to move all the addError to the attachmentChecks function in Subs-Attachments.php and avoid the double job done now. Does it make sense? :P
I split the template/theme crap because I know nothing about them and I like to keep my topics clean. (I can produce enough crap myself :P)
Your new years resolution is to be mean :P
I think it makes sense yes ... but we need the test monkey to chime in since I'm not sure what is currently being done with the attachments function in terms of additional refactoring. Makes no sense putting time in to code that may not be used or can't be merged in or just has to be rewritten to fit the new paradigm ... well other than for your own enjoyment :)
I've missed this discussion, sorry. Or rather, I missed the occurence here, but we had it already on github in fact. O:-)
Thanks for playing around with them. It makes sense, as far as I can see, to handle attachments errors in a separate class. My only issue is that it should be a subclass of error_context, but at the moment I don't have a good way to refactor context() for it (I was reviewing that code from web interface only atm). Methinks it's better if you pull in the code and we'll see if we tweak it further.