Thanks for the detailed review, Rob!

It's great to get this kind of in-depth feedback early on. If you don't
mind, I'd like to expand on some of the reasoning behind the choices so far,
and correct some errors. :)

Rob wrote:
> - Skin folders are named: plonetheme_sunburst_custom_images,
>   plonetheme_sunburst_custom_templates and plonetheme_sunburst_styles.
>   "custom" to first two doesn't seem to be logical. Better use
>   plonetheme_sunburst_images and plonetheme_sunburst_templates.

Agreed, this is just what ZopeSkel produces by default — and I don't touch
stuff I don't know how works. If anyone wants to change it and make sure
whatever registrations that reference it are updated, feel free to do it.
Last time I tried, my theme product was not functional anymore.

> - is empty but is called from the import_steps. If this
>   isn't used better remove it.
> - templates and in the browser folder seem to be unused and
>   be removed.
> - jsregistry.xml and viewlets.xml are empty, better remove them.

The idea is to have a final review at the end where we remove all the files
that aren't useful for the core theme. It's quite shocking to see just how
much crap is required in Plone 3 to add some simple templates. But yes, as
few files as possible is definitely the goal.

> - All css validated except main.css. The errors are caused by the use of
>   corners and rgba colors. Since this will become the default theme I
>   sticking to the current CSS standards and move these extra features to a
>   separate css file so the integrator can choose to use it or not.

This is valid CSS3, and is also what we're already using CSS3 properties in
Plone 3. I'm not sure whether the validator is showing regressions, or
what's going on — I'm pretty sure I didn't get errors on rgba properties
earlier, but I might be wrong.

There was one legitimate error (cursor property), which I have fixed. The
rest seem like validator software bugs. :(

> - member.css uses a lot of unneeded !important statements. These statement
>   css debugging a lot harder.

I know people have a reflexive dislike of !important, and I took great care
not to use any in the main style sheets. However, for the state coloring,
they are necessary, since you don't actually know what kind of elements they
are used on, and what other CSS is applied. The state color should always
take precedence when it's specified, so we have to use !important.

I agree with the principle of not using !important, but this is one of the
few cases where it is actually warranted.

> - Html validation failed with 20 errors and 32 warnings.

If you switch the validator to HTML5 and add the doctype as noted, there are
no errors in templates I tested. An important concern when deciding on
whether to adopt HTML5 or not was whether pointing the validator at it would
cause issues — but luckily it already supports HTML5.

>   - No doctype specified (as noted in the plip).
>   - No character encoding specified (should be the first meta).

These are valid concerns.

>   - Search form uses "name" attribute.>

This is allowed in HTML5.

>   - Label of searchbox not inside block element.

This is allowed in HTML5.

>   - History form uses input which is not inside block element.>

This is allowed in HTML5.

>   - All html5 tags are not recognized.

See above. ;)

> - The following issues do not meet the accessibility requirements:
>   - Not all images and symbols have a text equivalent (alt tag)

I couldn't find any from my casual testing. Which templates were these? Note
that we supply empty alt tags if the image has no usefulness to the meaning
of the page.

FWIW, it passes the accessibility tester at

>   - When javascript is disabled the "Display", "Add new" and "State"
menu's are
>     visible.

Good catch, thanks! This is a legitimate issue, but seems to be unrelated to
the theme. The theme CSS has dl.actionMenu.deactivated dd { display: none; }
— but it seems that the menu rendering code doesn't apply this class anymore
(or does it using JS). I can't really tell where the "deactivated" class
supposed to come from. I suspect the same bug exists in current Plone 3

>   - Some values are specified using 'px' where they should be specified as
>     relative value like em or %.

Right, there were still some px values left. Fixed.

>   - Names are used for color, where numbers should be used see:

That's not what that guideline says. It shows you a *technique* to help you
make sure that you can easily see that there's enough contrast by looking at
the percentage/hex value, but that doesn't mean that there's anything
magical about using hex/percentages over named colors. The few colors we use
that named (gray and red on white) have plenty of contrast.

Of course, when constructing an accessibility validator, it's easy to add a
rule like that to trigger a warning, which is a good idea, since a lot of
the named colors aren't good combinations (red on orange, etc). But in our
case, we're OK.

>   - Text like Description and discreet could use a bit more contrast.

OK, made them a little darker. Have a look now.

>   - Inline style elements are used, where classes should be used.

Where? I couldn't see any when testing a couple of different templates.

>   - For printing an href="javascript.print()" is used. An alternative for
>     javascript users should be available.

It's called the "print" menu entry in your browser. ;)

If you want, we can avoid rendering this link if you don't have JS enabled —
but I have removed it altogether from the default layout, and let it be up
to the integrator to enable this viewlet again, since print / send to are of
questionable value.

It's a requirement that used to be common in RFPs etc, but seems to be
eschewed in favor of Facebook/Twitter buttons these days — which are equally
annoying and not used by anyone — statistically speaking — either. It mostly
means "we are hip to social media" these days, and is mostly present on
sites that aren't. Can you tell I don't like them? ;)

>   - sup tags are used where css should be used. Sub and sup can only be
>     if the meaning of the content is different not for presentation's
sake. see:

The registered trademark symbol is supposed to be superscript, do you see
any other use that conflicts with this?

> - h1 - h6 tags should be used for document structure and no levels should
>   skipped. Like the current theme this theme also uses h5 tags without
using h2
>   till h4.

My goal was to change as little as possible of the existing markup. If we
introduce new tags for the h5s, we might break existing themes. I'm happy to
make this change in Plone 5 where we are doing more significant markup
changes, but I don't think it's worth being purist about it at this point.

> - Even more so then the current theme the contentview tabs and the
>   look the same. Contentview tabs make you 'leave' the current page and
>   formtabs keep you on the same page. This is confusing for users to see
>   different behavior for controls who look the same.

I'll beg to differ, sorry — they do the same thing, show aspects of a
content type, the fact that some reload and some don't is a different issue.
They are trying to express the same concept, and while the implementation
isn't ideal, they are expressing the same kind of grouping (although on
different levels). We're getting rid of these in Plone 5, so they will be
short-lived anyway. Nobody really wants two levels of tabs. :)

> - The PLIP states: "Keep the theme color-neutral (black, white, grays)"
but I
>   see green and blue are used which are not color-neutral.

Some colors are non-negotiable — you need blue links, and I reused the same
blue for the global navigation (just inverted). There's a lot of
documentation that talks about "the green bar", so in the interest of
keeping things simple, I added that color too. Those are the two only
colors, and they are trivial to change, and examples of how to do it are
provided in ploneCustom.css.

The point of the statement was to address all the other use of color, which
makes it hard to theme Plone to a particular visual profile. The two colors
used now can be changed by uncommenting two new values in ploneCustom.css,
and nothing else will conflict with a given corporate visual profile.

> - Building the theme using html 5 seems like a good idea but it does
>   some issues:
>   - The html doesn't validate.

See earlier comments. 0 errors if you set it to be HTML5. :)

>   - Since older browsers don't support the html 5 tags the html needs to
>     both the html 5 tags and the 'old' div tags making the html a lot less
>     clean.

The markup is pretty messy already, but I don't think we should try to fix
this until Plone 5 to keep things simple for existing themes.

>   - Only the main template is updated using html 5 tags. All other pages,
>     viewlets, portlets etc don't use the html 5 tag which leads to
>     inconsistancy.

Not sure what you mean here — the portlets are already wrapped in the
<aside> tag, are there any other obvious tags that you think are missing for

>   Building the theme using xhtml 1 strict seems like a more stable way to
>   Maybe add the html 5 in a next release (or make it a configuration

Apart from a couple of new tags in main_template, XHTML1 and HTML5 are the
same. I don't see the issue, and Plone has historically been and early
adopter wrt. these things as long as they actually work — which they do.
Browsers that don't understand HTML5 will just render as HTML4 (or XHTML,
should you choose to use that MIME type). We were the first CMS to use XHTML
and CSS2, and I'd like us to stay with the times. This kind of stuff does
send the right signal to developers, which is something we sorely need on
several levels. :)

Making it configurable is not an option as far as I can see, as XHTML is a
subset of HTML5, but not the other way around. It would also be hard to
maintain. Let's pick one. It's pretty clear which one has a future at this
point. :)

> - Like mentioned in the PLIP notes some features are still missing so
can't be
>   tested, like RTL, print, mobile etc.

Print and mobile have been added. Feedback welcome, they are both relatively

Thanks again for the fast swift review!

Alexander Limi ·
Framework-Team mailing list

Reply via email to