[Piwik-hackers] HTML cleanup

Thu Jun 26 01:52:24 CEST 2008

Hi Milian,
I reviewed your patch and commited to the SVN :-)
The patch you send was good except that line breaks were windows line
breaks, try to change your IDE settings so that you use unix line breaks.

> 1. Because of the ampersand issue (see above) I needed to patch some smarty 
> plugins to use `htmlspecialchars()`. Do you have an internal function with 
> possibly extended functionality for that which is preferred?
>   
Not as far as I know. I think that patching all the individuals
function, as you did, was the right thing here.

> 2. How can I make sure in `plugins/Home/templates/menu.tpl`, that the 
> `<ul>...</ul>` section is skipped for non-existing/empty $level2.name's ? 
> I.e. it's never good to have empty junk-tags around.
> I tried `{if $level2.name}` which did not work. This applies btw. to the 
> dashboard menu entry. This is one of the 18 errors left for the Home page.
>   
{if isset($level2.name)} ?

> 3. It's good practice to specify the language in the `<html>` attribute - how 
> can I get the current chosen language code?
>   
You can get it in the language array. However this is the full locale:
'es_ES.UTF-8' for spanish.
We could write a smarty helper function to get only the language part
from this locale.

You can display the locale using {'General_Locale'|translate} in the
template code.

> 4. You use the name attribute of the `<a>` tag in 
> `plugins/Home/templates/menu.tpl` and `.../menu.js` to store the AjaxURL. 
> Quick and dirty. Very dirty. Since the name attribute has quite some 
> restrictions, just like the ID attribute. I'd like to rewrite this by 
> generating a json object in `menu.tpl` which associates some proper menu-ids 
> with their respective AjaxURLs - would that be ok? This would eliminate the 
> last 17 errors from the Home page and bring us at least a valid HTML template 
> there.
>   
If you come up with a solution that is working, more elegant, and
concise, this is ok.

> But there is still much to do, since most of the HTML (even if valid) is still 
> a mess:
>
> - superfluous wealth of `<br />` tags instead of proper CSS styles
> - deprecated elements, i.e. center
> - senseless stuff like `<span id="h1">` instead of `<h1>`
> - presentational markup: `<b>` and `<i>` vs. `<strong>` and `<em>`
> - inline CSS - evil since not easily themable, use proper classes / IDs / 
> ancestor selectors to style them via external CSS files
> - mixture of JavaScript and Templates, it would be much more sane to separate 
> the two and put the JavaScripts right at the bottom of the page (i.e. right 
> before `</html>`).
> - possibly more I've spotted while skimming through the files and have since 
> forgotten
>
>
> So that's basically it for now. Comments? Patch review? Hate? Praise`
>
>   
We're looking forward to the next patches.
Your action to clean up all the existing markup is useful.
The change that will also be very useful for the community will be the
whole structure templates cleanup. I think that the objective would be
to be able to build a theme by simple CSS overwrite.

Let me know
Matt