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. Adding > "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. > - setuphandlers.py is empty but is called from the import_steps. If this file > isn't used better remove it. > > - templates and viewlets.py in the browser folder seem to be unused and should > 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 rounded > corners and rgba colors. Since this will become the default theme I suggest > 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 make > 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 http://wave.webaim.org. > - 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 releases. > - Some values are specified using 'px' where they should be specified as a > 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: > http://www.w3.org/TR/WCAG10-CSS-TECHS/#style-color-contrast 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 non- > 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 used > if the meaning of the content is different not for presentation's sake. see: > http://dev.w3.org/html5/spec/Overview.html#the-sub-and-sup-elements 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 be > 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 formtabs > look the same. Contentview tabs make you 'leave' the current page and the > 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 generate > 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 contain > 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 portlets/viewlets? > Building the theme using xhtml 1 strict seems like a more stable way to go. > Maybe add the html 5 in a next release (or make it a configuration option). 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 simple. Thanks again for the fast swift review! -- Alexander Limi · http://limi.net
_______________________________________________ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
