Freetag plugin code changes: help

[yellowled]

Revamping my blog template I (once again) looked at the (X)HTML code emitted by the popular freetag plugin. And, boy, didn't I like what I saw! The code emitted by this plugin is nowhere near standards compliant, it's a disgrace, and it's utterly gross, period.

Unfortunately, the PHP code of said plugin is way too much for me and my baby PHP, so I'm gonna need some help here. Let me walk you through my proposed changes.

1. "Defined tags for this entry: [tags]"

This is wrapped in a <div class="serendipity_freeTag">. I think it should be <p class="serendipity_freeTag">, since <div> should be used to group elements, not for marking up a single line of text. <span> would be an alternative, but <p> is clearly the better element in terms of semantics.

Yes, I could change that myself. But the content of this is a constant with a variable (%s) included, if that's the right term for that. I think this should be seperated into a constant and a variable. Plus, the value of the constant should be a plugin config option so that people can shorten it to i.e. "Tags:" or something like that.

2. "Related entries by tags: [entries]"

Same here. This is a div, the related entries are links, and all of it is "formatted" using <br />s. EOW! This is how the code should look like:

Code: Select all

<div class="serendipity_freeTag_related">
    <p>PLUGIN_EVENT_FREETAG_RELATED_ENTRIES</p>
    <ul>
       <li><a href="...">...</a></li>
       [...]
    </ul>
</div>

Again, the constant used should in my humble option be a plugin config option, people might want to change it to "Related articles" - and why shouldn't they? The ul could also be an ol, and it probably should have the new class plainList assigned to it to keep the design consistent.

3. freetag cloud (the tag cloud displayed in plugin view)

Man, this is a tough one. Both "Entries tagged as %s" and "Related tags" are marked up as <div>s. They should probably be headings, but it is next to impossible to determine the heading level here since it should be related to the rest of the page, which depends on the headings the template already uses.

This is one of the many reasons why I'd actually prefer this plugin to be smartified, but unfortunately, I can't do that either.

So the most likely suggestion for these two would be the same as above: Mark them up as <p> with the same class they use now. Again, I think these should be configurable.

Now, for the actual tag cloud ... well, it is kind of okay-ish the way it is now (<span>), but I guess it would be better as an <ul>. Of course, this needs quite sophisticated CSS for the cloud to look the same as now, so I'm gonna drop that suggestion (for the time being)

But how about the rest? Anyone got the time and the nerve to help me implement this?

YL

[garvinhicking]

Hi!

1. I am against using p instead of div. People might have styled the plugin using a div specifics CSS already, and also I don't hink it's wrong, because it's a container for different elements. At least, I don'T see any problem with using a div here, can you elaborate on what is problematic with a div?

I agree though to make content of this variable configurable, but basically I think the plugin should deliver all content inside variables already so that you can manually output everything from the plugin through specific Variables or constants?

I don't use this plugin myself, so I'm not too terribly deep into customization.

Regards,
Garvin

[yellowled]

1. I am against using p instead of div. People might have styled the plugin using a div specifics CSS already, and also I don't hink it's wrong, because it's a container for different elements. At least, I don'T see any problem with using a div here, can you elaborate on what is problematic with a div?

Well, define "problematic"

What I'm advising here is to make the code more compliant to (current) webstandards. That's not actually a change which will result in "Whoa, that looks awesome!" comments, but it's most definitely good in a greater sense, if only in the long run. Benefits of using standards compliant code include SEO, accessibility etc.

Now, the <div> element is supposed to be used to group elements, i.e. like this:

Code: Select all

<div class="container">
    <h1>A heading</h1>
    <p>A paragraph</p>
</div>

That way, h1 and p can be addressed via CSS together (in one CSS statement) using .container.

<div> is a generic container element, more precisely: It's the generic container element on block level (as opposed to <span>, which would be appropriate to group elements on inline level). It is most definitely not the right element to mark up a piece of text like this:

Code: Select all

<div class="whatever">This is my content.</div>

One would usually use <p>...</p> for this. This can be exaggerated (and is, in many cases) to a ridiculous extent:

Code: Select all

<div class="container">
    <div class="h1">A heading</div>
    <div class="p">A paragraph</div>
</div>

This is what we refer to as "div soup". This code misuses (one could even say: abuses) elements for a purpose they're not meant to be used for, especially since there are elements available which are better suited in terms of semantics.

Let me elaborate this even further: A <h1> carries a semantic meaning. It says "This text is supposed to be a heading, level 1", whereas <div class="h1"> has no semantic meaning at all. It is "bad" for a number of reasons, including SEO (search engines indexing documents by headings) and accessibility (most screenreader users use headings to skip through a document).

Yes, I know it's a bad example since it doesn't refer to the "<p> instead of <div>" we're talking about here. But still, a <p> is better than a <div> since it at least carries a little bit of semantics ("this is a paragraph of text"). It's (almost) the same as using .serendipity_msg_important instead of .red as a class name

As for

People might have styled the plugin using a div specifics CSS already

You mean like this?

Code: Select all

div.serendipity_freeTag { ... }

Yes, that's a possibility. And frankly, in my humble opinion this would be an example of bad CSS coding But you're of course right, this is possible. I have to admit I have no real solution for this -- but still, this shouldn't be a reason not to make s9y better ... tough call.

Again, this would be a great argument for simply (optionally) smartifying everything

basically I think the plugin should deliver all content inside variables already so that you can manually output everything from the plugin through specific Variables or constants?

I have to admit I have no idea what this means. Sorry

YL

[garvinhicking]

Hi!

Do you have the s9y book? I think especially for the freetag plugin I documentet all variables and constants that should allow you to address all custom markup situations.

About "div soup". Isn't it like this that <p> is not allowed to be nested? Like a <div> within a <p>? I always thought that the freetag plugin container might contain other div elements...and just my own feeling is that a div is more approrpriate for a taggroup than something that has a semantic meanting of a paragraph....

$0.02

Regards,
Garvin

[yellowled]

Do you have the s9y book? I think especially for the freetag plugin I documentet all variables and constants that should allow you to address all custom markup situations.

Of course I do I'll have to check that, but my concern here is really not to improve the code in my own blog but in s9y in general. That's just my passion, probably comes with the territory

About "div soup". Isn't it like this that <p> is not allowed to be nested?

That's correct, but there wouldn't be any nested <p> elements in the code I have in mind.

I always thought that the freetag plugin container might contain other div elements...and just my own feeling is that a div is more approrpriate for a taggroup than something that has a semantic meanting of a paragraph....

There is nothing wrong with the code here. But it could be improved. It probably doesn't matter anyway since you're not into the plugin enough and my PHP skills are nowhere near good enough to change it myself

The thing is, it keeps getting more and more frustrating to "hit walls" just because it isn't as simple to modify the output of certain plugins as it is with most of the core ...

YL

[garvinhicking]

Hi!

I could put this on my "December vacation" table, if you remind me?

Regards,
Garvin

[onli]

Hi
Just wanted to drop the note that I'm working on that. I have a working and smartified version here, but am thinking about organizing the code in a better way. Hope I don't hurt anyone if I say that functions like displayEntry(&$eventData, $addData) are not as they should be - my code, which tries to change as little as possible regarding the existing logic, don't profit from that.

The changes will however introduce params for displayTagCloud() and displayMetaTags(), use an unnumbered list for relatedEntries when using standard html-output and search one step deeper when searching for neighbored tags (which strengthens the effect of parent-tags, tags that connect other tags, because their connections will be sooner noticeable).

Dropping the current version here. Or do you want to make a cleanup anyway Garvin or view the risk as too high and I shall simply integrate the changes but leave the code as it is? Then this version should almost be my finished commit, but of course needs testing and polish.
sincerely

PS: Upload isn't working, so: serendipity_event_freetag-3.10.1.tar.gz

PPS: I wrote a note, a code-expample, into the Readme, describing how the introduced smarty-variables could be used - and by the way listing them.

[garvinhicking]

Hi!

Maybe yellowled could give some feedback; I won'T have time before december to check into this...

Regards,
Garvin

[yellowled]

I already promised to do so via Email, but I'm quite busy myself right now. Might take a few days.

YL

[yellowled]

Might take a few days.

Or not

Okay, overall this seems to be working okay. I stumbled across what I considered a missing .plainList for the related articles, but then I realized that my test blog is still running 1.4.1, so it doesn't have a style_fallback.css yet.

I really don't get the description for the Sidebar template -- am I supposed to enter a template into the corresponding input box? If so, how?

As for the event plugin -- if I use PLUGIN_EVENT_FREETAG_EXTENDED_SMARTY, what am I supposed to set the dropdown box right before it in the configuration to? Yes, No or Smarty? I'm guessing Smarty, but it's really not that obvious. Or doesn't it matter at all?

I tried testing the Smarty code from the readme, but it doesn't work -- does this maybe require a recent beta (still running 1.4.1 here)?

YL

[yellowled]

As for the event plugin -- if I use PLUGIN_EVENT_FREETAG_EXTENDED_SMARTY, what am I supposed to set the dropdown box right before it in the configuration to? Yes, No or Smarty? I'm guessing Smarty, but it's really not that obvious. Or doesn't it matter at all?

I tried testing the Smarty code from the readme, but it doesn't work -- does this maybe require a recent beta (still running 1.4.1 here)?

No, it doesn't -- I figured it out, the aforementioned dropdown box needs to be set to Smarty. I think this should be mentioned explicitly in the plugin config. With this setting, your example code works like a charm. Haven't really looked at the sidebar plugin, though.

I have to say, I love this I think the description variables don't even need to be configurable this way -- I can simply use something like

Code: Select all

<p><span>Tags:</span> {foreach from=$entry.freetag.tags.tags item="tag" name=tagline}{$tag}{if not $smarty.foreach.tagline.last} • {/if} {/foreach}</p>

(or substitute "Tags:" by a template lang variable) to get a whole different output for the tag line.

Awesome. I have no idea whether this has any downsides or if you need any further feedback, but: Great job, onli!

EDIT: While we're at it: The freetag plugin also emits bad code in the backend for the "manage tags" area. Can we please change

Code: Select all

<div style="border: 1px solid #000; padding: 5px;">

(line 1589 in your current serendipity_event_freetag.php) to have a class so admin template designers can change the styling? That would be great. (I'm working on an alternative admin template right now, which is a PITA as it is, but stuff like this is just wrong, period. I know it's not your fault, of course )

YL

[onli]

Hi
Thanks for the fast feedback. I didn't change anything to the sidebar-plugin and I'm also not really sure what template should point to. The footer-option really matters and must point to smarty, as you already noticed - the description will point that out.

I will change the logic to emit the whole smarty-variables all the time, so an additional check have to be made in the template to prevent the dispaly of related entires at the frontpage - but this was wished a few times.

Of course adding a class is no problem, but what about the inline-css? Think that should remain, or at least bulletprroof should be updated (I haven't checked yet how important these settings are).

[yellowled]

I didn't change anything to the sidebar-plugin and I'm also not really sure what template should point to.

Well, so it seems to be old, and I just didn't notice it before, so I thought it was a new feature. LOL.

I will change the logic to emit the whole smarty-variables all the time, so an additional check have to be made in the template to prevent the dispaly of related entires at the frontpage - but this was wished a few times.

No idea what you mean by this

Of course adding a class is no problem, but what about the inline-css? Think that should remain, or at least bulletprroof should be updated (I haven't checked yet how important these settings are).

Erm, this refers to the backend, so I guess it would be better to include inline styles for backwards compatibility with older template which don't have an admin template. Then again, I'm always in favor of losing any inline styling I guess this should be Garvin's call, but after all, admin template can always overwrite the inline styling or remove it using JS ...

YL

[onli]

I will change the logic to emit the whole smarty-variables all the time, so an additional check have to be made in the template to prevent the dispaly of related entires at the frontpage - but this was wished a few times.

No idea what you mean by this

At the moment the smarty-variables for related entries won't be emitted if more than one entry is shown. I will change that, but only if extended_smarty is enabled. Maybe anyone can use them.

[yellowled]

EDIT: While we're at it: The freetag plugin also emits bad code in the backend for the "manage tags" area. Can we please change

Code: Select all

<div style="border: 1px solid #000; padding: 5px;">

(line 1589 in your current serendipity_event_freetag.php) to have a class so admin template designers can change the styling?

I just now realized that there is a <p> in line 1773 of serendipity_event_freetag.php with a similar inline style, but w/out a class. This should get a class, too.

YL