Skip to main content
Topic: So that we remember (or at least I don't forget :P) (Read 9292 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

So that we remember (or at least I don't forget :P)

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.
Bugs creator.
Features destroyer.
Template killer.

Re: So that we remember (or at least I don't forget :P)

Reply #1

Lovely rrefactoring, I must say.

Quote from: emanuele – 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).
I saw that! And it worrks like a charm! Nice interraction ;)

 Feature Cat nods approvingly, or purrs, hard to say at times.

QuoteIf I missed something let me know.

The other ~100 possible errors, to be handled by error_context  O:-)

 Feature Cat runs.
(don't say it.)

Re: So that we remember (or at least I don't forget :P)

Reply #2

 emanuele is working on them...one by one...
I just committed the verification code and RtM.

In the meantime:
Code: [Select]
		// 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:-)
Bugs creator.
Features destroyer.
Template killer.

Re: So that we remember (or at least I don't forget :P)

Reply #3

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.

Re: So that we remember (or at least I don't forget :P)

Reply #4

I'm looking now at the attachments errors...
What would you think about having a separated error box for those?
Bugs creator.
Features destroyer.
Template killer.


Re: So that we remember (or at least I don't forget :P)

Reply #6

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:

Code: [Select]
	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:

Code: [Select]
	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.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: So that we remember (or at least I don't forget :P)

Reply #7

@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.

Quote from: Feature Cat – 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.
 emanuele 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).
Bugs creator.
Features destroyer.
Template killer.

Re: So that we remember (or at least I don't forget :P)

Reply #8

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)."
Last Edit: December 29, 2012, 04:58:35 pm by Antechinus
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: So that we remember (or at least I don't forget :P)

Reply #9

 TestMonkey thinks it works just fine either way, as long as I can still read the result. O:-)
The best moment for testing your PR is right after you merge it. Can't miss with that one.

Re: So that we remember (or at least I don't forget :P)

Reply #10

 emanuele 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.
Bugs creator.
Features destroyer.
Template killer.

Re: So that we remember (or at least I don't forget :P)

Reply #11

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.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: So that we remember (or at least I don't forget :P)

Reply #12

Yep, I think I was thinking to remove one of the two classes, but I forgot... lol
Bugs creator.
Features destroyer.
Template killer.

Re: So that we remember (or at least I don't forget :P)

Reply #13

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
Bugs creator.
Features destroyer.
Template killer.

Re: So that we remember (or at least I don't forget :P)

Reply #14

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)
Bugs creator.
Features destroyer.
Template killer.