Add a new theme to D7 core: Bartik

Nice work Jen! subscribe :)

As great as this is, due to feature freeze it won't make it into core. Keep the development up and we can look at it for early inclusion in D8.

New themes aren't API changes, let's give webchick and Dries a chance to look at this.

oh, nice. subscribe.

Nice! I like the layout -- much more modern than say Garland's. It is also nice to have a theme that has some extra regions. In general, it also feels right. It feels like it has the right level of sophistication, and the proper balance between simplicity and complexity.

My only real criticism is that the colors are a bit depressing (for lack of a better word). I'd prefer to have a "happier" initial experience, if that makes sense. Maybe it is a matter of changing some of the colors? I certainly want to avoid design by committee, but I figured I'd share my initial experience.

Relative to other Drupal themes, I'd give it a 9/10. Relative to other non-Drupal design, I'd give it a 7/10. It is certainly very promising, and I think it could be a significant step up from Garland. I definitely look forward to see how this plays out/evolves over the next couple of days.

Nice theme !
Will the primary navigation support active states ?

Will it have support for the skinr module ?

Bartik is pretty!

Very nice!

Some small things:

• Buttons don't look like actual buttons at the moment, more like text fields.
• I'm not sure if it's wise to style the form item descriptions italic. By doing this, emphased pieces of text can't be separated from the other text anymore.

Other than that, I'm really impressed. It feels like a more 'mature' Garland. It looks more like an actual 'site', something I don't think Garland does very well. (Garland is like half site, half admin.) If this theme would support the color module, I think it would be the perfect replacement.

There's a lot of positive things to say about this! Good job, nicely done.

I do agree with Dries on the color aspect of it, but as has already been pointed out, support for the color.module would render this issue a non-.... issue. :)

Would also like to know about any grid or other system or subtheming stuff that might be relevant here?

Wow!!

We did a lot this release to focus on improvements that make Drupal 7 out of the box more palatable to designers. I think this could be the capstone. I love the origin of the name, too! :D

Looking at the code, it seems this has a couple of nice things:

1. It looks like this is based off the 960 grid system. A lot of designers are familiar with this and the fact that Drupal ships with a core theme based off of it would show off its flexibility immediately.
2. Its .info file is longer than 4 lines! This makes it more of an 'advanced theme' that, if built into core, can give folks a taste for what's possible.

I only got like 4 hours of sleep last night, so I'll have a look more in-depth later, but for now A+++ WOULD SUBSCRIBE AGAIN!1!

+100000000 for this theme going into core. Like right now.

and another +100000000 for this being the default (instead of Garland, which could probably still ship with Drupal....just because it's been there for a while, and there have been some really good improvements on it)

and another +100000000 for webchick getting some sleep here pretty soon ;)

This looks great, typography is good, layout and overall feel is good, details like comments / user pictures are good too. Didn't review code at all, would be interested to see how poll/forum and other badly behaved modules look in it.

We haven't had a new default theme since Drupal 5.0 was released three years ago. That's a really, really long time. We also removed pushbutton, bluemarine etc. this release, which is great, but was on the assumption we wouldn't ship with only one front-end theme. Installing with Seven then getting Garland feels a bit backwards too.

So I'd love to see this as a new default theme, Garland needs to stay in as not the default at this point.

I also really like that this doesn't scream Drupal as much as either Bluemarine or Garland do - it looks really usable for an actual website, so should also play nicely with others if we have a theme chooser in the installer and/or new themes in Drupal 8/9.

+1 for getting this in if at all possible. I'd be interested to see what Mark Boulton has to say about this, since he was rallying for a new frontend theme in #drupal-themes a couple weeks ago.

If this theme supports color.module, it will kick some serious ass, no question about it. I'm very, very encouraged by what's been done here, and I think that the presence of a multi-region theme in core will do a lot to open casual evaluators' eyes to the fact that Drupal can go way beyond 'header, column, content, column, footer' layouts.

Is this being developed in a CVS sandbox or somewhere we can checkout? I'm very encouraged by this!

I think I have some work for you ;)

ps: found this while playing with the collapsing button of advanced search

A couple first impressions from the tweetesphere -

http://twitter.com/gusaus/status/7697483815
http://twitter.com/TopNotchThemes/status/7697954491

Amazing work, Jen! Bartik into core would be major accomplishment for Drupal.

Personally I like it. A bit similar to Garland in gestalt, but much cleaner.

Stylesheet suggestion: Put stylesheets into a /styles or /css folder, and break out the major color settings into one separate stylesheet so the n00b themers can at least override only one stylesheet to color up their derivative theme to their tastes.

Nice work!

Another glitch

I like the look very much.

The sidebar to me seems a bit too thin when compared to many other two column themes - it seems to be more like a three column theme with one column turned off instead of a native two column design.

(Many sites needing to allow adverts, there is a standard ad square of width 250px (as opposed to the 18p0 that is currently allowed for), so having it wide enough to fit such adverts in the content may be an idea to consider... but that would make the sidebar 70 pixels wider.)

as has been mentioned before, the top (primary?) menu items do not seem to have a hover state.

A third place of potential improvement is just below the top menu - where it goes into the main page background, it seems a little too simple. Maybe give the menu a bottom border of a few pixels (that then maybe the tabs of the menu can blend into)?

All in all, I am not a designer and many of these ideas may be plain stupid, but I think they are worth exploring.

/is-not-a-designer-and-has-no-design-sense so feel free to ignore.

I just made a blog item, and visited it...

Would be even better with div#page { background-color: white; }, and not this simple transparent one.

Edit: attempted to retag, but somebody beat me to it. Ignore.

I really like it and hope this gets into Drupal 7 core. Well done!

One small critique though, and this is just my opinion: to me it looks a bit odd to have a larger corner-radius on the tabs (8px) than on the sidebar blocks (4px). In my opinion, the tabs are a bit too rounded and would look better and more consistent if they used the same 4px radius as the blocks.

Here's how it would look with a consistent 4px radius:
http://img.skitch.com/20100113-893yqdwj4g5biwnwn8bjh55k7r.jpg

I like the clear looks it has. I also think it needs a bit more work.

Active menu items is a must imho. This can be easily done by changing:
#navigation ul.links li a:hover { (line 278 of style.css) to:

#navigation ul.links li a:hover,
#navigation ul.links li a.active {

I also like the typography, although i personally like Arial based fonts for the main content text (headers are fine in serif), though it's just a personal preference.

This needs an accessibility review, tagging as such. For example, I feel the form buttons are a bit too dull to see, imho they should come up more, since the cursor stays the same, and there is no hover action.

I love the #footer-columns styling, very nice. Again, needs accessibility review, might be too little contrast there, but I personally like it =)

Looking at the markup

• I can't help to notice that it prints <div class="region region-page-top"> </div> and <div class="region region-page-bottom"> </div> with no content in there. I think the theme should check if there's anything to print there before printing out these container divs.
• In the region #triptych, each block is contained inside two extra divs, <div class="section"><div class="region region-triptych-middle">, which are not being used for anything, and which aren't necessary, so these should be taken out.

I like this a lot. I have one piece of feedback:

I like how the links in the demo site appear to be centered. This is visually striking since centering text or other elements in a container usually doesn't look good and is sometimes frowned upon by designers. But it works here. Looking at the CSS I see that the node links are not actually centered but are indented using margin:

#block-system-main #link-wrapper { margin-left: 236px; }

I think this could be a problem in the long run. Some sites have many links in this area, and those sites would have 236px less room for them. Two ways to deal with this would be centering the text in the container or giving only a slight — 10-15px — margin-left to #link-wrapper.

What do others think about this?

Cheers,
Alex

Great job so far!

I downloaded the zip file. I'd like to see a couple of changes including:
- layout folder for css files that are grid/layout related.
- I feel weird about a credit back to a company in core. I'm cool with a credit being in the code, but I don't like it being displayed in the description. I would like to see that credit removed from the Description in .info.
- Where's the rest of the copyright for 960.css? it's licensed under GPL, but who owns the copyright? It seems as though this file as been edited by Spry Soft?

Unsure how I feel about, but want to highlight...:
- Should line 95 of node.tpl.php be a preprocess function instead of having this formatting in the tpl.php file? Moving it would show people how template.php works. But maybe that just complicates things unnecessarily? What do other people think? The snippet that I'm referring to is:

          <?php
            print t('published by !username on !datetime',
              array('!username' => $name, '!datetime' => $date));
          ?>

The theme isn't blue enough! :P

/me ducks

Great job. So happy to see a new theme.

A quick point: as is the case with Garland, if this theme makes it into core, then it will be used/hacked as a starting point for custom themes for years to come. As such, it's really important that the theme is coded cleanly and beautifully, and sets a good example for other themes.

With that in mind, I'd vote to at least move it out of 960gs in favor of more semantic class names and less unnecessary CSS. Thoughts?

this is awesome! good work, will be testing and send some screenshots of admin

Nice. A few items to tidy up and explore

1. breadcrumbs unstyled and unimpressive
2. tables unstyled and unimpressive
3. I can't find a pager. Hopefully they look consistent
4. perhaps turn on taxonomy terms so we can see those term pages.
5. create a content type with text/number Fields so we can see how that looks.

- Should it be a framework theme or just fancy replacement of Garland? 960gs and reset etc suggest former, theme structure (it's not a subtheme based on abstract 960 builder theme) suggests latter.
- Will we now have 2 reset.css' in core?
- What about NineSixty theme what was once planned for core? Why that was no-go and this is go? Because NS was not providing fancy skin subtheme?

My take on this: I am blown away by aestetics of this as rest of the thread, but the theme's structure and reusability as base theme (960 part) needs serious thinking.

But if you care about Drupal7 alpha wow-effect committing this NOW and pushing Garland non-default is our last chance. We still can re-architect theme's structure later.

Nice! I like.

Re: 960.
The 960 framework looks very cool, but it falls down as soon as you want to do anything reasonably complex such as having a column subdivided if that column is being made in several templates (think Views).
http://www.1kbgrid.com/ has a better system for subdividing columns in that there is no need for an extra class on the containing column.

Does it work with Color module? It should.

A few thoughts...

This theme should go in without it working for color module. While I love color module, we have a short window and that should not hold it up. I would hope we can commit the theme first and then follow-up with color module support if someone wants to do it.

@kika - I don't think a framework theme is a good idea right now. We need another reasonably good looking theme in core. The offer is for a non-framework theme so lets look at that. This isn't the place to suggest the type of theme.

Plus, the theme system in D7 changed a lot. I think it will take a little time using D7 to figure out what a framework theme should look like. For example, many of the cool things Zen did are now part of core theming.

I like the idea of using the 960gs for this core theme. It's used as part of the theme and not as a framework in core so it is ok. One bonus over other grid frameworks is that we have access to many people who know 960 really well and the person (Nathan Smith) who created and maintains it.

If there is concern over the grid and how it scales we may want to strip out the grid and do the positioning in a very basic way. But, I still like 960.

One thing to watch out for is if the file name of a css file is the same as the file name of a core or module css file it will replace/override the core/module file.

Haven't had time to review the theme, but the comments have me really pumped. Subscribing.

@mfer, even if it's not a "framework" theme, it's still going to be used as one. Look at the number of sites that have used Garland as a starting point. We need to code it like it is going to be adapted, even if that's not its real purpose. Besides that, even if it will never be adapted, we still should use it as an example of how Drupal themes can output semantic and clean code, since Garland failed at that.

Maybe I'll be the only one that thinks this, but I think the color module is a bad idea for this theme. It will just complicate things unnecessarily IMO. Isn't the color module one of the top reasons why Garland is a bad theme to start with?

I would much rather see theme a simple theme setting that adds a class to html or body to control the color schemes. It could even randomly switch between a few different ones, if you wanted to get cute. If the colors are separated out like Laura suggested in #24, it wouldn't be hard to achieve/understand. This is how I do themes that need multiple color schemes. It works well and is easy to manage.

An extra thought; one drawback of the color scheme is that it lessens the difference between the front-end (i.e. Bartik) and the back-end (Seven) a little bit. Both the front-end and the back-end are black-ish themes. This might not be a big deal but it struck me as I was playing around with the theme.

I was about to say the same as spencerwyatt in #31, so here's a +1 for a 4px radius on primary links.

Also, I'm wondering which browsers are targeted in core themes. I noticed that some details are not optimal in Opera (using version 10.10). Some of those details simply depend on CSS3 rules (like border-radius) that Opera doesn't support yet. Others can possibly be fixed. See the attached screenshot: radio buttons look awkward and the search field is 2px higher than the search button next to it.

Internet Explorer will suffer from the same square edges as Opera.

I concur with #49. We do not want to waste precious time/energy/resources/code/etc. making tiny aesthetic details like this look the same in all browsers, as long as it is functionally equivalent and has no obvious visual discrepancies (like, say, the left sidebar floating in the middle horizontally in Opera 11).

For the record, I'm a fan of doing rounded corners in some browsers and square in others using CSS3. The one thing I would as is that "-khtml-border-radius" is used alongside the others so we can have rounded corners in more browsers.

I'm REALLY digging this design! Like everyone else chiming in, this is exactly what the doctor ordered to cap off the UX improvements in D7. I especially like that the navigation is more conventional and clear than Garland, making the design useful for many types of sites. With color module integration all can be made happy regarding the "flat" look.

The attached annotated screenshots are meant to nit-pick the details to bring further polish. Please take this constructively. Design is largely subjective so @jensimmons should have full veto on anything listed. No single point should be a seen a commit blocker (Seven had many detailed missed when committed).

@#49:
I am completely in favor of using CSS3 for rounded corners. If you got the impression that I consider the lack of rounded corners in Opera (and IE for that matter) a bug: I apologize.

Still, I'd like to read your opinion on #31, because a 4px radius on the primary links is more consistent and (IMHO) looks much better.

Really nice work with this! Let's get this done! :)

Color module support

I've just added very basic color module support for Bartik. Just to get it up and running. Some tweaks needs to be done to the color palette of the theme, to make it work smoothly. There is also still work to be done one the preview file and functionality.

I'll come back later with a proper patch and a full implementation, if someone won't beat me to it ;)

/dixon_, NodeOne

Edit: I've attached the whole theme here. But basically, what I've done is to add the color/ directory and the template.php file. And then only some small tweaks to the color code formattings in style.css. As I said, I'll come up with a proper patch later.

As an alternative to color.module support - which presupposes someone has the capability to create nice functional colour schemes and, as has been said, complicates things - what about providing one or two sub-themes that just have different colour palettes?

I think that if this eventually will become a new standard theme, that replaces Garland, it needs to support the color module. Otherwise the color module wouldn't do anything out-of-the-box. An that would only be confusing for users, I think.

I also think that this specific theme is suitable for a good color module integration. It is clean, doesn't have many images which makes the implementation quite clean it self. But as I said in #54, the theme needs some palette tweaks to make it the perfect fit.

I love this theme. I love the progressive enhancement. +1 for active menu styling in comment #52.

#58: it does put active classes on the li's. See screenshot below. Am I missing something?

Nice work here. Hopefully this is gonna shake some designers off the tree... C'm on I know you are out there.

One more theme with core ambitions is in the making, though only in layout state right now, will be there by next week.

I'm also working on a theme, but I would need at least several days to get the whole design done and several more to code it. How much time we have?

@jensimmons: Do you need some help with this theme? I actually enjoy doing this kind of work and would be willing to roll up the sleeves.

Nice work, @jensimmons! I love the attention to contrast and typography.

I'm sure plenty of us are tired of Drupal lagging behind other platforms (like WordPress) in the design department. So glad to see some cutting-edge designers are helping to change that. And showcasing CSS3 as well.

960gs is a solid CSS framework and my company built dozens of Drupal themes with it in 2009. It's very expandable and IMO is a perfect springboard for people to jump off into theming Drupal. If we have nitpicks with it, I'd think we could easily fork it since it's GPL+MIT. But it's advantageous if we use 960 as-is since it's well supported, documented and in use by other platforms.

color.module support would be a requirement to put this in core, that seems clear.

960gs is a solid CSS framework and my company built dozens of Drupal themes with it in 2009. It's very expandable and IMO is a perfect springboard for people to jump off into theming Drupal.

On the other hand, the "other" message could be being sent by using a separate framework for a core theme sent is that the core style and theming from the modules is not good enough...

Looks AWESOME!!!!

Small Things that I noticed:

• The primary tabs do not have an active state. I'd love to see this go all out with the menu having a "hover", "pressed" and "active" state. (Best known example doing this is the apple menu on apple.com)
• Errors aren't styled. (Tried voting in the poll and got an error, that's when I noticed.
• Site looks weird when the site height is much, much smaller than the browser window height. (Again, got that when I tried voting in the front poll, got an error and a very short page.)

@nbz, on the contrary, 960gs is only a structural framework, i.e. laying out columns and other large areas. Core and module styles can live happily within that structure.

My point is that, Garland for example has very convoluted CSS to define its page and column structure. Newbie themers have a difficult time since Garland is usually what they start with. If core came with Bartik as the default theme which used ninesixty as its base theme, newbies would easily be able to tweak Bartik to their needs since all the weird floats, negative margins, etc. are taken care of by the base theme.

If we should have a chance at getting this into core, we must adjust the theme to the color module. If I understand things right, you can only have 5 adjustable color codes in the theme. It's important that the implementation is really smooth.

Adding a new tag, looking great BTW, cant wait to try it out.

found jareks issue

On setting Bartik to default:

    *  Notice: Undefined index: 0 in system_theme_default() (line 436 of /Users/joachim/Sites/7-drupal/modules/system/system.admin.inc).
    * Notice: Trying to get property of non-object in system_theme_default() (line 436 of /Users/joachim/Sites/7-drupal/modules/system/system.admin.inc).

edit: nm, in core.

Two thoughts:

If you make the background black (as proposed in the issue list on the demo page) that would cause problems the default behavior of many wysiwyg editors. Often, they re-use the main stylesheet which would cause black-on-black.

Mobile devices are used more and more, so wouldn't it be good if this theme would have a mobile stylesheet as well?

Impressive punch-list, Jen :)
We should design the functionality in a way every new core theme (in a very optimistic outview) reuses the same code.
Jarek is working on some of the same stuff, so no need for duplication.

I'll try to keep track of the proceedings in both themes so maybe we can prevent duplication and make the coding as awesome as the looks.

@marcvangend - I think it would be a good idea, as it would make many module maintainers think about what happens on a reversed colour theme. I have run in to a number of modules that have been near-unusable on themes which don't have a white background, as the module maintainers test it with core themes and assume that every theme will be dark text on light background.

Things has to move forward quite fast here, if we shall have a chance at getting this into core. We need to attach a full real patch here, or at least upload the code as a contrib theme here on d.o so we as a community can work on this together. Core maintainers and core developers needs to have an easy way to look at the code so they can review it easily.

@jensimmons I guess you are sitting on the latest theme code? Can you create a usable patch from it? After that I can add my color module support from comment #54 and other people could also start hammering down your list at http://bartik.milkweedmediadesign.com/node/2 .

We need it as a patch, or at least as a contrib theme to be able to work on it fast.

@wretched sinner: that's also a way to look at it, but in the case of wysiwyg, I don't agree. Usually, modules should not break because of the chosen theme, that's true. However for a text editor this is deliberate behavior: in absence of more specific information, it re-uses the theme css to approach the site's design. That's not bad programming, that's an educated guess.

Aren't admin tables set to be 100% width in core CSS? At least they were in D6. Too many themes override this; they shouldn't.

@dixon_: github?

+1 etc.

Bartik looking great and solid feedback in the comments. Excellent work all around.
It would be great to see this land for core. From chatting with jensimmons in IRC I know a git repo is in the making *right now*. So that should surface soon enough.

I also soon hope to see a reworked version of http://bartik.milkweedmediadesign.com/node/2 posted as a "meta-issue" outlining the plan for landing this as the new default core theme.

Git repo:

http://github.com/jensimmons/Bartik

Enjoy!

Wow this is fantastic! This is really what D7 needs to completely shine. Typography! White space! Progressive enhancement! I swoon. Awesome work.

Awesome work!! I like it! And thinking this is *so* necessary for D7 release (would be kind of sad to ship with Garland IMO)...

Jen, I bet more than a few are willing to help out to get this 100%... do you think you have specific stuff you want us to help with?

I wonder if @webchick and @dries have said/would say how good a chance this has to get into core...?

I've started to integrate color.module here: http://github.com/smerrill/Bartik based on dixon_ 's patch.

Wasn't there supposed to be bullets with the unordered list (from the demo site)? IE7 (on XPSP2)

Given that so many people love this theme (including myself), I think we should try to get it included in Drupal 7, possibly as a replacement to Garland. I'll brainstorm with webchick on a deadline. Before we do, it would be good to have jen propose a deadline.

Great theme!

One small request: could you pad or indent the unordered list so the bullet isn't to the left of all the other text on the page? Many themes have the same problem, and it always looks like a bug to me.

Hanging indents....

Personally I think they may look good in print, but are fraught with problems on the web, and in particular in a CMS. They run the risk of bumping into and overflowing from containers. Also, there is the unresolved (to my knowledge) question of what should happen on nested lists.

I think it's up to site designers if they want to try them, but not for Drupal core.

+1 for getting this in core (and possibly default) as well... Here is a quick initial review:

1. Please add Skinr support!
Skinr is taking off with many of the top themes/theme frameworks supporting it these days (Zen, Fusion and Fusion based themes like Acquia Marina & Prosper, Studio)

2. -1 for Color module. I would recommend going with a simpler method such as Jacine suggests.

3, Foot menu styling: This current appears to be a vertical list menu and I think it would be better as a horizontal list.

4. +1 for mobile styles as well.

Nice work Jen and everyone!

For those of you saying that this theme shouldn't support the color module, don't forget that the hope is that this theme will hopefully replace Garland as the default theme, and if the default theme doesn't support the core color module, then there's very little reason to keep including Color module in core. If we need to get rid of the Color module, then that's a different issue, but as long as it's there, the default theme should support it, IMO.

@mcrittenden I haven't forgotten anything. There are reasons I feel other methods should be explored. The "because this theme is going in core it needs to support the color module" argument doesn't fly with me. Since when is color module support a requirement for core themes? If I missed the memo, and that is the case, this is a sad day.

I've seen quite a few people end up in #drupal-themes looking for help after trying to use Garland as a starting point for their own theme. They are usually trying to change something simple, only to realize that the way they know how to fix it (with CSS) doesn't apply because the color module has taken over. Worse, there aren't many people that actually understand it well enough to help (myself included). These people are usually told starting with Garland was a bad idea and they should have used a different theme, like Zen. This is true, but frustrating nonetheless. The fact that help is needed from developers for implementation should be a red flag.

Aside from that, who actually uses the color module to implement color scheme options in themes outside of a small amount of contrib themes? A class on body or html does the trick nicely, and IMO gives the designer better control over the color schemes.

That said, I am not totally against having color module support in Bartik. In an IRC discussion between myself, Jen Simmons and Steven Merrill, an option was discussed to have color module support in a subtheme of Bartik. That would be a better move IMO. At the end of the day, this is just my opinion, but I stand by it regardless of what happens with Garland or color module.

As for Skinr, since it's not in core, and Bartik is going to be a core theme, I think any support for it will need to live in contrib.

The question of supporting color.module brings up an interesting question. Should the default Drupal theme be designed to be hacked on? If so, this may dictate more than just the usage of color.module. For example, comments would on where to change things and the layout of CSS files would make a difference.

To say don't include color module because of people hacking on the theme doesn't sit well with me. To say a goal of the theme is to be hacked on and provide a base for people to learn and that color module doesn't support that well makes more sense. But, having that goal also dictates other things in the theme as well.

So, is that a goal of the theme? If so, what else in the theme is working towards that goal?

To say don't include color module because of people hacking on the theme doesn't sit well with me. To say a goal of the theme is to be hacked on and provide a base for people to learn and that color module doesn't support that well makes more sense. But, having that goal also dictates other things in the theme as well.

I didn't say not to include support for it for that reason, and that is not the only reason. It's one of them. I also think people are going to hack at it regardless of whether it is the intended goal of the theme or not, just like Garland.

Anyway, what I'm getting from this thread is that core themes need to support the color module, which is good to know (even if I think it sucks). So, I'll shut up now. :)

Thanks Jacine for the well-thought out responses. A few replies:

The "because this theme is going in core it needs to support the color module" argument doesn't fly with me. Since when is color module support a requirement for core themes?

It's not, but IMO it should be a requirement for the core DEFAULT theme, because otherwise that will officially make the color module the only module that does nothing out of the box (I think?).

They are usually trying to change something simple, only to realize that the way they know how to fix it (with CSS) doesn't apply because the color module has taken over.

Again, this sounds like an argument against the color module itself which is a separate issue. We can't decide to have the default core theme not support color.module because of a fault of the module, because that's in effect saying "even though this module is in core, it really sucks and we, Drupal core developers, don't even want to use it."

an option was discussed to have color module support in a subtheme of Bartik.

The same problem of color.module doing nothing out of the box would still apply. Plus, adding a whole new theme (even if it's a subtheme) just for color.module seems like it would just add a layer of confusion and cruft. Unless you meant that the subtheme would have a different look as well?

Anyway, what I'm getting from this thread is that core themes need to support the color module, which is good to know (even if I think it sucks).

I pretty much agree that it sucks. Color module is a pain to work with, and I wouldn't be sad to see it go. But it's not going anywhere anytime soon :(.

I was actually thinking, this theme should be designed (as a goal) to be hacked on by others. A lot of people start with the default theme and hack away at it. That's where they start learning. So, why don't we take that opportunity to do something with the default core theme to help them go down that road in a good and peaceful way.

I like the idea of color module. If we want that for this theme I like the idea of adding it as a subtheme. That can extend the default theme and show off how you would do that. Provide a before (default theme) and after (sub theme). This would help people learn to use color module as well.

Thoughts?

I'm hesitant to start questions about The Big Purpose Of This Theme; it feels like there are a lot of subtle issues that jensimmons wants to finish up in the short term to get it "ready for commit," and for the time being it satisfies one of the biggest needs: a default core theme that is fresher and more flexible than Garland.

I'd love to see it support color module (if only because it's a very simple and easy way for non-designers to make it look a bit different). In a perfect world, I'd love to see it provide a theme settings page as well: the Fusion theme by TopNotchThemes uses that page to give users a choice of several font-families for the body text and headlines. The Singular theme, by DevSeed, gives users a file upload field where they can upload a custom background image for the site.

If we're going to spend energy making this theme customizable, I'd actually prefer that we invest in a few of those features rather than trying to tug it towards the role of a 'base theme for the masses'. A clean, simple base theme would be a big boon but I would hate to bikeshed Bartik because we want it to do double-duty...

Just my $.02 as a non-designer who must occasionally design. ;-)

I agree with eaton. We did a lot of work this release to make Drupal core itself a good base theme. See Stark.

If we can do color module support as a sub-theme, that also makes a great followup patch. I don't think it should be a requirement for an initial commit. Also I don't think we should cram in features which use core stuff just because it's there - if it makes sense on its own merits, that's fine, but Garland would still be in core and still recolorable.

If we can do color module support as a sub-theme, that also makes a great followup patch. I don't think it should be a requirement for an initial commit. Also I don't think we should cram in features which use core stuff just because it's there - if it makes sense on its own merits, that's fine, but Garland would still be in core and still recolorable.

I agree that the extras I was discussing shouldn't be commit-blockers. I just meant that I would prefer to see our default theme feature more easy configuration options, as opposed to turning into another Zen/Mothership/etc base theme. Perhaps I misunderstood what others were talking about -- if so, my apologies. :-)

I agree with comments above that color module provides a quick and simple way to make a site look distinctive.

The problem with Garland + color is that even once colorized, Garland still looks... like Garland.

I think Bartik has a more generic look, and so different shades of it will look more distinct.

I've not looked at the way it's built, but I imagine it will also more easily stand having dimensions pulled around a bit (eg making the header area bigger/smaller, sidebar widths), which Garland didn't tolerate.

So with color providing a co-ordinated way to change the color scheme, and a few dimension changes, you've very quickly got something that doesn't look totally like out-of-the-box Drupal.

@eaton, sorry I wasn't directly replying to you. I fully agree not trying to make it into a proper basetheme is a good plan. People don't use Garland as a base theme, they just hack on it - making sure Bartik is a good hackable theme with less of the pitfalls of starting with Garland is very different from trying to do a core base theme.

After sleeping on it I woke up finding myself agreeing with eaton. This theme will be a face of Drupal. Most people will just use it (not hack on it). Making it a great theme for users would be better.

Then again, this is the @jensimmons show. This is just my $0.02.

I agree with all of #109, and I speak as a developer who cares about good CSS and HTML, and has sometimes filed issues on other modules regarding use of tables for layout! ;)

See #693504: Color.module does not support more than 5 colors and has hard-coded labels for a question about the viability of adding a minor (backwards-compatible) API change to color.module, since we may want a few more than 5 changeable colors.

Somewhat OT, but let's avoid hyperbole like "don't care at all" and "failed miserably." Remember that Drupal is the lump sum of contributions by people who give freely of themselves, with what knowledge they possess and what time they have to dedicate. For some people that knowledge is deep underlying guts of the system, for others it is a deep understanding of web standards, and for still others it's how to put documentation together in a clear and concise manner.

Real, actual, human beings worked very hard on Garland, and Color module, as well as modules out there that output non-standard markup. Ignorance about best practices needs to be combatted with education (and patches), not hurt feelings.

Back on-topic, Jen, have you given any thought to a deadline you'd like to achieve? I'm getting a bit nervous about how scope-creepy this is getting, and want to make sure that we have ample time to fix inevitable clean-up bugs after.

I definitely support the idea of letting users define their own color schemes, but this doesn't have be done via the color module. The CTI Flex theme (http://drupal.org/project/cti_flex) lets users define custom colors by directly entering the hex color code or through a color wheel if the color picker module (http://drupal.org/project/colorpicker) is installed. I think this way looks very clean and easy to use. Perhaps someone can have a look at the CTI Flex theme and see if that approach can be adopted here?

Also, I really like the suggestions rickvug gave in comment #52. These are the nuances that make a theme stand out. :)

Re: #115: As has been said before:
Color.module is not the best solution out there for making themes color-adjustable. The situation simply is, though, that we have color.module in core. Therefore I don't think it's really an option for a core theme to establish a pattern of "evading" a core module. We're simply stuck with color.module for Drupal 7.

I have integrated the color module to a theme before and found that not being able to change the labels or add more than five colors (thanks for the patch stBorchert!), was a huge limitation. However saying that this module vomits over your css is not accurate.

The color module won't change anything below this statement:

/*******************************************************************
* Color Module: Don't touch                                       *
*******************************************************************/

So if all the colors you want to change are kept above that, and colors that are supposed to be constant (no matter the color scheme) are kept below that, then there's no vomiting going on ;)

I would like to know more about Steven Merrill's progress in color module integration, because maybe I could help out. Cheers!

I think that all this hate towards Garland is a big mistake and due to this misunderstanding the development of this theme, that I really like, is taking the wrong path. Let me expand on this and please forgive my english.

1. Garland is a beautiful theme.
Everything (from typography, colors and design) is in the right place. In my opinion Bartik, at this stage, is still beyond Garland. It may look better because we are all bored to death of Garland after staring at it every time we install Drupal; but please don't let this feeling pollutes your judgements.

2. The scope of the default theme should be to work out of the box, not being the base theme for small tweaks.
Garland and the color module are there to give people a great theme that you can tweak using a UI, not having to deal with code. Consider that one of the main point of this release is to make the life easier to newbies. (even if you don't like the idea)

3. I love web standards and ultra clean html as everybody, but we should all understand that writing the HTML for Drupal is not like writing HTML for a static page. There are a lot of use cases to take into account and this inevitably leads to a more convoluted code. If you want a super-clean HTML in Drupal the only solution is to write your own theme for your specific project. (and yes, for sure, Garland has some - too many - extra divs that are there just for cosmetic reasons, but maybe we can just correct that)

4. The use of a css framework like 960 (that I love and use sometimes) clutters the css even more. It's not a solution in this case but a problem. You force people to learn another framework on top of css. If someone really wants to change the width of some regions is much easier to write well commented css and make it easy to change a couple of numbers than having to understand yet another framework (and here we can make better than Garland avoiding to use negative margins that makes tweaking more complex).

5. Looking at the files (not the code), it seems to me that Bartik overrides some templates. In my opinion the default theme shouldn't. Doing that is like saying that Drupal default templates are wrong. If they don't work for the default theme why we wrote theme as such? The same for default regions. On the other hand I support the use of theme specific settings to replace the header image, change fonts and such. Unlike regions and template overrides, those don't rewrite Drupal standard code but provide extra functionality and are specific to the theme.

7. Still, I really like the theme. It lacks a little bit of personality and the depth of Garland (you can see this when the site is still empty, and since this is the case with every fresh installation it's something we should consider ). Garland with all those subtle 3d effects, gradients and transparencies is a little bit overwhelming but even really effective. Bartik is for sure more versatile but even of less impact (at the moment).

What I see at this stage is a really good theme for core but not as default. I think this would work well as the theme of choice for newbies to do small tweaks and code customization to fit their needs, as an option. In the end this doesn't mean I don't appreciate the job that has been done. Being in the middle of trying to build a theme for contrib. my self I understand the difficulties of writing the one that must suit every need and please many people.

P.S. Talking about sites that don't have to look the same in every browsers, I agree only if we believe that those little details we'll miss (rounded corners, in this case) aren't important for the overall design. (Screenshot in IE7 attached) (moreover as a standardista I'd like to listen people opinion on using vendor specific properties, - like those of webkit, in example - unless they're there to prevent specific bugs)

For over a month, I've been meaning to get back to completing a full explanation of how to evaluate color contrast. But that might not happen until you want this theme done. So in the interest of saying something useful, I'll point out that many Drupal themes have too little contrast between text and background. We seem to be trapped in pastels. There is a great tool called Color-A that can guide designers toward the selection of palettes in which each color used is paired with colors that have sufficient contrast for people with impaired vision to see the text. (You do have to know the hexadecimal code for at least one of the colors you want to use. If you have the color but don't know its hex value, you can use the Colour Contrast Analyser to find it.)

And let me point out that this is a case where more is not necessarily better. A relatively rare visual impairment makes it difficult to see text when the contrast is too high. So achieve enough contrast to comply with the guidelines in WCAG 2.0, but do not try to achieve the highest contrast possible. (These tools do tell you whether the selected colors have sufficient contrast according to WCAG 2.0. The advantages of Contrast-A are that you can use the tool to build a complete palette and, if any color pair lacks sufficient contrast, the tool shows you how to change either color to obtain a pair with sufficient contrast.)

I wish I had time to explain more completely, but I hope this steers you in the right direction. And I am really glad to see this theme. Great job, jensimmons!

5. Looking at the files (not the code), it seems to me that Bartik overrides some templates. In my opinion the default theme shouldn't. Doing that is like saying that Drupal default templates are wrong. If they don't work for the default theme why we wrote theme as such? The same for default regions.

I'm not sure I agree with that reasoning.

The ability to create a custom template file for an individual theme is one of the fundamental strengths of the Drupal theming system, and one of the first things that needs to be done when building a new theme. Forcing us to adopt a one-size-fits-all approach to regions, template files, and so on seems counterproductive. Shipping a theme that leverages that without cluttering every other theme with a list of unneeded regions or needlessly specialized markup seems like a great idea.

There's nothing to stop another designer from putting together a pure CSS skin for core's default templates: even Garland doesn't go that far, providing a template.php file and a page.tpl.php file. If anyone is willing to do that, it would be pretty cool. Bartik doesn't have to be that particular theme, though: I'm pleased that we would have one in core that actually DOES show off what can be done with overrides...

> 1. Garland is a beautiful theme.

Yes, it is. And yes, we're all a bit tired of it because we see so much of it, and we may in 3 years be tired of Bartik ;)

But I also also think that some of Garland's elements (the pointy white triangles on primary links and the cut-away section for the breadcrumbs) are a bit too distinctive, and so whatever you do to it, you still have something that fundamentally looks like Garland. Beautiful, but not well suited to purpose.

I get the impression that with Bartik, someone who quickly wants to make a site can just drop in an image into the header, play about with colours, and have something that looks reasonably personalized.

@Eaton I'm not saying that drupal default template files and regions should fits every theme in core but just the default one. Otherwise there's no reason to have those defaults. If we feel the default theme should ship with different templates and regions, I believe we should change Drupal default files and regions not override them.

We already have a theme in core which exposes the default templates, it's called stark.

The default templates are designed to be easy for CSS-only themers to extend. The default theme is supposed to look nice when you install Drupal and show off some features. These are two different tasks. It's not impossible that someone will submit a CSS-only theme for D7, and when that happens, there can be a discussion about whether that should be committed, and whether it should be the default or not, but currently that's not on the table at all.

Jen, if you're planning on a 1/31 date, which sounds good to me, we need a patch uploaded here asap so that people can get started reviewing the code, etc.

Jacine indicated on IRC that I perhaps haven't been as urgent about this as I need to be. Here goes.

I am extremely concerned that we are heading into February, and I am not seeing fervent back and forth patch reviews on this issue. This is the surest sign to me as a core maintainer that rapid progress is happening and there is a team of people to drive this home. Right now I see no team. I see something that looks nice on the surface and a lot of comments that amount to basically "+1!".

If there is such activity happening in an off-site repo (such as github) to better facilitate collaboration, then great, but there absolutely has to be periodic patches/summaries posted here so that the rest of us can keep up with (and contribute to) the progress. Our core patch reviewers are here, not in github, and it's imperative that these reviews be tracked transparently for all to contribute to.

I am also very nervous that we will end up with 3-4 nice looking, but non-core-worthy themes because of resource allocation issues, and this whole initiative will end up getting bumped to Drupal 8. We have a very limited number of front-end developers contributing to core at all, and those people are currently stretched around at least three core themes plus whatever else they were already working on. If we don't see a huge influx of new front-end people giving reviews, I think we might need to cut our losses and try to get just one of these in rather than three. :\ I really don't want to do that, because this would be totally amazing if we could pull it off, but we need critical mass in order to do so and I am not seeing it.

So what I need to see, absolutely as soon as possible, are actual code reviews, and lots of them on this issue. That only happens when we start posting interim patches. So please let's get some code in this issue!

I'd like to help but I'm not sure what the priorities are. Help? I could just download the code from Github and pick something from the list (http://bartik.milkweedmediadesign.com/node/2) but it would make sense to spend time on the most urgent issues.

A few things from Jen's list:

# Add a tool for uploading a header image. With help text about image sizes.

Surely something for core rather than a single theme -- and too late for core this time around.

# Create theme setting to hide the site title and slogan (by pushing it off the page with text-indent:-9999px; not getting rid of it altogether)

Ditto; though perhaps doable?

I'll start reviewing & testing as soon as a patch shows up.

@webchick Ok! Ok! :-D

I've been talking to Jen since July about this theme in one form or another. And I've already committed to helping polish it up and make it core-worthy.

There's still a bunch of work to do, but to soothe your nerves, here's a patch off the most recent work over at http://github.com/jensimmons/Bartik It doesn't yet include the color-module branch that Steve and Jen were working on last weekend, just the stuff in the master branch.

And the images tgz file should be put in the themes/bartik directory before expanding it.

subscribe

Might as well let the testbot have a go.

I will chime in and say that we got it to work with core's existing color module and labels, so while an API change to make color.module would be nice, it would not necessarily be required.

scribing

This theme odes not have background-color... so look ugly if browser's background is different from white

I'll patch my d7 head install and give a full code review, but I just wanted to say that API change to make color.module would be so awesome! Please please please webchick make an exception for #693504, and please anyone who has ever intended to integrate a Drupal theme with the color module chime in on that issue. Our themes deserve a more flexible color module, and the changes are not drastic.

Bartik from github is great.

Patch in #130 has no screenshot, and gives me a heap of error messages when enabled. Though those could be because I also tried the color module branch and I now have my variables polluted with that...

The more I see this theme the more I like it :D

The Sidebar Second region doesn't work, whatever I put into it.

New (and real) screenshot attached

Here is a patch with all the latest changes from jensimmon's, smerills (color module), and joachim's (clearfix) git hubs. At least I think I got them all.

From now on I would strongly recommend to continue the work here on d.o with real patches! That way core maintainers, reviewers etc. more easily can see whats going on and review the code. I don't think they have time to check out stuff from git hub.

I also made a huge walk through of all the files and fixed a lot of code style issues to make it even more core worthy :)

• Added color module from smerrills repository (think it was based on my initial patch?)
• Removed code that was commented out
• A lot of code style changes (whitespace, linebreaks, blank line at end of file etc.)
• A lot of comment style cleanup
• Removed credits (that doesn't belong in core, unfortunately)

I also attached a new image pack that shall be placed in the bartik folder. It includes the screenshot from #144.

Ops! I somehow lost the preprocess functions in template.php that makes the color module work. A new patch is attached below.

You'll also need the image pack from #144.

Okey, it's too late here... This one will work correctly ;)

Strange... Let's try that again.

color.module support is awesome!
Here's a screenshot of the blue lagoon preset.
Two issues I found: The logo isn't colorized. changing body text and base colors doesn't seem to work (everything else does).

Talking about typography, it seems to me that the site name font is too small. Moreover I'd like to see Arial in place of Helvetica Neue in the meta, tags and links tags. Even if I love Helvetica, at this small size Arial is slightly more readable (more space between characters) and very similar. Attached are screenshot from Safari on a Mac (in the second one the site name is set at 240%, too).

P.S. Is there a way to add inline images to issues referencing attachments? I tried but the html has been stripped away

Re: typography -- after spending way too much time researching the Arial/Helvetica debate recently, it looks like it's still the safest to put Arial first for any small (body) text. Helvetica at small sizes becomes nearly unreadable for the ~7% of people who actually have Helvetica installed on XP, unless you have gone in and explicitly configured the font smoothing. Given the intended widespread audience of this theme, I think it should go the safe route.

> From now on I would strongly recommend to continue the work here on d.o with real patches! That way core maintainers, reviewers etc. more easily can see whats going on and review the code. I don't think they have time to check out stuff from git hub.

You mean patches that don't include the images, and are a PITA to make with CVS fakeadd?
As opposed to github, where it took me seconds to branch and submit my proposed change, and you can download the whole theme folder in a snap?

@joachim You don't need to do all the work here on d.o. But we need to regularly submit patches here, and consider this queue as the "head" or main bransch. Otherwise reviewers have no chance to keep up to what we are doing, and where we are doing it.

I will gladly help with the merging of fixes from your github account, and create CVS patches for those. It's actually not that hard when you have the tools setup for it. So keep up the good work and we all will get this done together! :)

Not to jump the gun, but it might be worth asking Dries or webchick to create the bartik folder and add the images in CVS as soon as this starts to look commit-worthy (haven't reviewed code yet so no opinion either way on whether it is or not). Then it's just adding files, which is a lot easier than messing with directory creation and image gzips, plus reviewers only have to apply the patch to get everything working. That would still allow the code to be properly reviewed before it gets committed, but without some of the overhead for testers and patch re-rollers.

Should I be opening new issues against this theme now or wait until it hopefully gets into core?

In order to get the color module to change colors, I had to disable and re-enable the theme. After doing that, this part seemed to work fine. I really like the dropdown thing instead of the tabs for edit and stuff. The forums still need some attention. I also can't get any blocks to show up in the second column region.

If the block in the first footer column doesn't have a height (doesn't have content) the entire footer collapses, hiding content in the other three footer columns. The same thing happens if no blocks are in the first column. I observed the same thing in the triptych area.

Environment
Ubuntu Linux, Chrome Browser

The previous color problem that was resolved by re-enabling the theme came up again, so that isn't just a quirk in the patch process or something.

In other news: My ability to make websites ugly has been enhanced by Bartik's color module support. :-)

@dodazzi, the sans font in #149 looks the same, I think they are both Helvetica.

Helvetica is one of my favorite typefaces for print, but while it looks great on Mac (Apple's version is optimized for small sizes) Windows renders it very poorly compared to Arial, due to the mysterious nature of ClearType. Until that is fixed, I think Helvetica should be listed after Arial in font-family definitions. Windows still has 90% of the OS market :-/

The Blue lagoon preset looks awesome!

@carlos8f
That's exactly what I meant: replace Helvetica with Arial, because Arial renders better at small size.
The screenshot are correct. Look at the "read more" link. You'd notice that it's a little bit more spacious in the Arial version.

@dodazzi, I'm not sure about the "read more" text, but tail on the "a" in "Drupal" seems suspiciously like Helvetica in arial.jpg :)

Glad to see some actual action on this issue. The proposed deadline looks good to me. I'm not sure we should remove Garland -- I still think it is a very solid theme that is on the same level as Bartik (except that us regulars are bored with it).

I'm not sure anyone's said we should kill Garland -- still ship with it, just make Bartik the default in its place.

@carlos8f
Just to clarify.
You're right, the site name is still Helvetica, in my screenshots. As I said I replaced Helvetica with Arial only where the font size are small: in the meta info, taxonomy and links.

Ok, so there is obviously some contention about the fonts used.

Now, can we please review the code? This includes things like:

- Is the code formatted consistently, according to the Drupal Coding Standards?
- Are the CSS rules written optimally?
- Is the output W3C compliant?
- Is there PHP code in tpl.php files that needs to be moved to template.php?
- Is the code in template.php well-documented and easy to understand?

I'm going through the code and doing a review right now. Will report back in a few with results.

@webchick, and all others

As I mentioned in #144 I walked through and reviewed/patched a lot of all those things already.

As discussed with jensimmons over IRC recently, we'll have another shot at this tomorrow.

This is more of a coding style review than anything else since that's all I could find...the big stuff seems to be pretty well thought out. I'm going to ignore all 960.gs related stuff since I've already stated my preference to have that removed.

+description = A flexible theme with multiple regions, written with well-formed code. Suitable to be used for building your own theme. 

We might want to think of a new description here. Pretty much all themes have multiple regions, and hopefully all core themes should have well formed code (if they don't, then that needs fixing). Also, "Suitable to be used for" doesn't really make sense. How about just "Suitable for"

+regions[triptych_first] = Triptych first
+regions[triptych_middle] = Triptych middle
+regions[triptych_last] = Triptych last

Can we think up a different name for these? I had to look up the word, and I'd assume others would have to too. How about Triplet first/middle/last or something?

+<?php
+// $Id$
+
+// Last conformed to D7 Head - v 1.18 2010/01/07 05:23:51 webchick

What's the deal with that?

+ *   desired parameters on the $comment->changed variable.
+ * - $new: New comment marker.
+  * - $permalink: Comment permalink.
+ * - $picture: Authors picture.
+ * - $signature: Authors signature.

Space before the permalink.

+    <?php print $picture ?>

Missing a semicolon (might be missing in the default in comment.tpl.php but either way needs to be fixed in this theme).

+      <div>
+        <?php print render($content['comment_form']); ?>
+      </div>

Can we get rid of those divs?

+  <?php if (!$label_hidden) : ?>

If I'm not mistaken, the colon needs to not have a space before it, even though that's the way it is in the default field.tpl.php, though I could be wrong.

+?><!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN"
+  "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-1.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="<?php print $language->language; ?>" version="XHTML+RDFa 1.0" dir="<?php print $language->dir; ?>"
+  <?php print $rdf_namespaces; ?>>
+
+  <head profile="<?php print $grddl_profile; ?>">
+    <?php print $head; ?>
+    <title><?php print $head_title; ?></title>
+    <?php print $styles; ?>
+    <?php print $scripts; ?>
+  </head>
+  <body class="<?php print $classes; ?>" <?php print $attributes;?>>
+    <div id="skip-link">
+      <a href="#main-content"><?php print t('Skip to main content'); ?></a>
+    </div>
+    <?php print $page_top; ?>
+    <?php print $page; ?>
+    <?php print $page_bottom; ?>
+  </body>
+</html>

Did you need to override html.tpl.php? I couldn't find any changes from the default. Also, if you're going to override it, seems like you'll want to remove the space before "<?php print $attributes;?>" in the body tag to keep consistent with the rest of the theme's templates.

<a href="<?php print $base_path ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>

Semicolon after $base_path (in maintenance_page.tpl.php)

+            <?php if (!empty($title)): ?><h1 class="title" id="page-title"><?php print $title; ?></h1><?php endif; ?>

Let's put breaks in for the if statement here to keep it under 80 chars.

+            <?php if (!empty($messages)): print $messages; endif; ?>

I'd like line breaks and new <?php ?> tags here as well just to stay consistent.

+        <div id="main" class="column"><div id="main-squeeze">

Line break and indention here as well please. Not sure if this is a standard or just a preference of mine, though.

+          <?php
+            print t('published by !username on !datetime', array('!username' => $name, '!datetime' => $date));
+          ?>

Line breaks are awkward here.

+    <?php
+      // We hide the comments and links now so that we can render them later.
+      hide($content['comments']);
+      hide($content['links']);
+      print render($content);
+    ?>

And here as well, though not quite as bad.

+<div id="page-wrapper"><div id="page">
+
+  <div id="header"><div class="section container-12 clearfix">

I'd prefer line breaks here as well, although again it might just be personal preference.

+    <div id="messages"><div class="section container-12 clearfix">
.....
+    <div id="featured" class="region"><div class="section clearfix">

Page.tpl.php gets a little div-tastic. That's just an example. Are the .section divs necessary here? Can they be bundled in with their previous divs instead?

+        <?php if ($title): ?><h1 class="title" id="page-title"><?php print $title; ?></h1><?php endif; ?>
+        <?php print render($title_suffix); ?>
+        <?php if ($tabs): ?><div class="tabs"><?php print render($tabs); ?></div><?php endif; ?>
+        <?php print render($page['help']); ?>
+        <?php if ($action_links): ?><ul class="action-links"><?php print render($action_links); ?></ul><?php endif; ?>

Again, I'd prefer some line breaks here, especially for the if statements.

+        </div></div> <!-u- /.section, /#triptych-last -->

"u" in comments

+      <?php print theme('links__system_secondary_menu', array('links' => $secondary_menu, 'attributes' => array('id' => 'secondary-menu', 'class' => array('links', 'clearfix')), 'heading' => t('Secondary menu'))); ?>

I could use some line breaks in these, for example after the commas in the array declaration.

+  if (!empty($variables['page']['triptych_first'])
+      || !empty($variables['page']['triptych_middle'])
+      || !empty($variables['page']['triptych_last'])) {
+    $variables['classes_array'][] = 'triptych';
+  }

Could probably drop down to two space indentions instead of four.

+  // Hook into color.module

Needs a period.

+function bartik_process_html(&$vars) {
+  // Hook into color.module
+  if (module_exists('color')) {
+    _color_html_alter($vars);
+  }
+}
+
+/**
+ * Override or insert variables into the page template.
+ */
+function bartik_process_page(&$vars) {
+  // Hook into color.module
+  if (module_exists('color')) {
+    _color_page_alter($vars);
+  }
+}

Do we need to do this in both preprocess_page and preprocess_html now? That's a bit of a bummer if so, but obviously outside the scope of this issue.

+#header #block-search-form { /* Search block when in the header region. */
....
+#header #block-user-login { /* Login block when in the header region. */

Not sure those comments are needed since it's pretty obvious what the declarations are targeting.

+#navigation {
+  padding: 5x 15px;
+}

5x

+  font-family: Helvetica Neue, Helvetica, Arial, san-serif;

This is used throughout...needs to be sans-serif right?

+#comment-wrapper h2.comments {
+  margin-bottom: 1em;
+}
....
+#comments .comment {
+  margin-bottom: 20px;
+}

Should probably use ems or px, not both.

+  background: #fafafa url('images/triptych-background.jpg') repeat-x top center;

No quotes needed, also doesn't it need to be ../images? Same error is repeated a few times.

+  font-family: Helvetica Neue, Helvetica, Arial, san-serif;

Helvetica Neue needs quotes around it (this is repeated a few times but is right a few times as well)

+.sidebar .block {
...
+  background-image: -webkit-gradient(linear, 0% 0%, 0% 95%, from(rgba(247, 247, 247, 1)), to(rgba(240, 240, 240, 1)));
+}

How about -moz?

Why does the theme come with its own field.tpl.php ? I see no difference with the original.

Most of the code style points in the code review above make sense to me.

I'll leave the CSS people to justify the DIVs in places like the tabs as I'm sure there is a reason for them.

I just want to say that 'triptych' is a perfectly good word and it describes the effect nicely.

Looks like mcrittenden beat me to the punch, but here's some more stuff to ponder.

The first thing that I noticed was that the directory name for the theme is "Bartik". To be consistent with all of the other files and directories in Drupal, this should be changed to "bartik", I think.

bartik.info:

• There should be a newline at the end of this file
• I think there are some region naming issues. I had to Google "Triptych" to see what it actually was. I think we could probably use a more common word here. It's probably not that big of deal, but was slightly confusing at first.

block.tpl.php:

• I know it's just a development note, but line 4 (And one of the neighboring empty lines) should be removed so that a patch can be rolled without a lot of hassle.
• Code is clean and conforms to coding standards.
• I might be missing something, but is there a reason that you are defining a block.tpl.php? Looking at themes/Bartik/block.tpl.php and modules/block/block.tpl.php, the only difference seems to be that the Bartik copy has that development note. I think this template file can be removed.

color/color.inc:

• Need a new line before 5.
• Lines 7 and 8 look horrible. I know that those lines are correct syntactically, but could we maybe put spaces between the commas and the next color value or would that completely destroy color module support?
• White space at the end of 8
• White space on 15
• For lines 29 and 30, could we simply say
  

    'slices' => array(),
  

  rather than splitting it out onto separate lines? You do that with line 25, so it would be good to stay consistent here.

• There should be a newline at the end of this file

color/preview.css:

• This file is presently empty. Do we need a /* $Id$ */ inside of this or is it ok simply being empty?

comment-wrapper.tpl.php:

• On a subject completely unrelated to this theme, we really really really shouldn't have module-specific code in the core template files. Line 34 seems like it should read something like this:
  

    <?php if ($comment_form_title_visible): ?>
  

  The variable name could probably use some work, but you see what I'm getting at? Then, forum module could just turn off visibility for the forum node type. I dunno. Just throwing stuff out there.

• Line 33 should be split into two lines, I think:
  

  <div id="comments-wrapper">
    <div id="comments" class="<?php print $classes; ?>"<?php print $attributes; ?>>
  

  The rest of the lines would then need some more indentation. Line 46 would need split as well. I think it looks a lot more clean like that. We don't really have any coding conventions for the markup yet (http://drupal.org/node/223584), so this is debatable.

comment.tpl.php

• Development note needs removed to roll patch
• Please remove line 64
• Lines 87-89 should be indented two more spaces

css/960.css

• Oh man, this file is a mess. I know it's not something that was written for this theme, but can we make this at least conform to coding standards please? There are tabs everywhere, and there should be /* $Id$ */ at the top

css/layout.css

• Spaces between attributes and values, please. Problem lines: 4,7,8,9,10,11,15,16,17,18,19,20,25
• Please insert a newline before 22.
• There is a large comment block from 29-87. Is there a reason that this is still included? Can we get rid of it?

css/reset.css

• Please insert a newline before 9, 23, 27, and 31.
• I think 13 and 18 can probably be removed.

css/style.css

• I know that we all appreciate the hard work you've put into this theme jensimmons, but I don't think 3-9 can go into core. I think a MAINTAINERS.txt entry would be in order, though.
• Please insert a newline before each new css rule (examples: Line 16, 19, 39, 45)
• It seems like color module stuff should be at the end of this file.
• 27-29 are particularly confusing. Why is this there?
• Line 37 will be ignored by browsers that do not support CSS3, right?
• Please remove lines 12, 32, 82, 243, 304, 318, 343, 422, 480, 508, 537, 615, 638, 661, 668, and 719. Additionally, the comments immediately preceding each of those lines should be changed into a block quote similar to themes/garland/style.css, lines 3-5.
• Please remove code that's been commented out. Lines 62, 344-346
• Please be consistent on your font-family declarations (use quotes on Helvetica Neue throughout). 376, 379, 415, 450, 543, needs quotes around Helvetica Neue, and an 's' in san-sarif
• Comments at the end of rules should be moved to the line right before the rule. Example: comment at the end of 174 should be moved to the line above
• Line 237-239: Why? Can the comment be updated to explain?

field.tpl.php

• Development note needs removed
• I think this template can be removed too. The only difference is the development note.

html.tpl.php

• Development note needs removed
• I think this template can be removed too. The only difference is the development note. However, is this now required for a page.tpl.php or something?

maintenance-page.tpl.php

• Development note needs removed
• I think this template can be removed too. The only difference is the development note. If the maintenance theme is set to Bartik, it should use the default maintenance-page.tpl.php from the system module

node.tpl.php

• Line 93: "published" should probably be capitalized.
• Line 123: We don't want to use the id attribute here because /node displays multiple nodes on one page, which causes validation errors. Class would probably be a better choice here.

page.tpl.php

• Line 64 has some whitespace at the end.
• Line 168: The comment is a little messed up. An extra "u" got in there. XHTML + RDFa validation error.
• After the other issues are fixed, we need to make sure that this validates as XHTML + RDFa. Having a theme in core that is not standards-compliant would be bad.

README.txt

• This file can probably be removed. There's no content, and anything that would go here would probably be better off in the d.o documentation section.

template.php:

• Code is clean and conforms to coding standards.

Whoops.

body {
  font-family: Georgia, "Times New Roman", Times, serif;
  line-height: 1.4em;
  font-size:0.8em;
  word-wrap: break-word;
}

I'm not sure about body font size. I would either respect user preferences and do not specify it at all (it's just like setting body font-size to 100% or 1em) or ignore user preferences altogether and specify my own fixed value.

Let me give you an example. There are generally three kinds of users:
1. Geeky users - they know how to configure default font size in their browsers. They also know how to use zoom functionality but they are always annoyed when they have to do it.
2. Normal users - they have never touched browser preferences. So their body font size is left to default value which is 16px. They are relying heavily on zoom functionality provided by browser.
3. IE6 users - just like normal users, but their legacy browser does not allow them to zoom on some pages. Besides they are evil.

If you don't set body font size (which is equal to setting it to 100% or 1em):
1. Geeky users will be happy because fonts will match their preferences
2. Most normal users will be sad because of the big fonts (16px)
2. Most IE6 users will be sad because of the big fonts (16px)

If you set body font size to 13px:
1. Geeky users will have mixed feelings - this is the font size most of them would expect, though you ignore their preferences
2. Normal users will be happy - this is the font size they would expect and they can use zoom functionality
3. IE6 users will have mixed feelings - this is the font size that they would expect but they can't zoom it

If you set body font size to 0.8em:
1. Geeky users will be very sad (font size will always be smaller than their preferred size)
2. Normal users will be happy - this is the font size that they would expect (0.8em * 16px = 13px) and they can use zoom functionality
3. IE6 users will be happy - this is the font size that they would expect and they can use zoom functionality

Conclusion: there is no difference between 13px and 0.8em for normal users, but fixed units are more friendly to advanced users while fluid units are more friendly to IE6 users. There is no perfect solution, but considering the fact that IE6 usage is already far below 10% (if you exclude asian countries from statistics :P), should we still discriminate advanced users for the sake of IE6 users?

regions[triptych_first] = Triptych first
regions[triptych_middle] = Triptych middle
regions[triptych_last] = Triptych last

regions[footer_firstcolumn] = Footer first column
regions[footer_secondcolumn] = Footer second column
regions[footer_thirdcolumn] = Footer third column
regions[footer_fourthcolumn] = Footer fourth column

Those could be replaced with just two dynamic regions. Take a look at ADT basetheme. I'll try to implement it when I'm done with my theme.

Can we please start moving these conversations about specific topics (like typography, bug reports, etc) into individual issues? There is too much going on in this issue, and we need action items that can be assigned to people.

I started making some issues for Bartik today, from Jen's list. They are located here: http://drupal.org/project/issues/search/drupal?issue_tags=Bartik

To create a new issue, do the following:

1. Look here and make sure it doesn't already exist
2. Create a new issue here: http://drupal.org/node/add/project-issue/drupal
3. Make sure the version is set to 7.x-dev
4. Component should be "Other" for now
5. please tag it "Bartik" so we can find it

All the action is happening on Github at the moment. Here is a quick recap:

• jensimmons has been working on CSS and typography stuff. She also removed the 960 grid system from the theme and replaced with custom CSS.
• dimitrig01 has implemented some theme settings and other PHP logics. Also some security fixes.
• I have been focusing on code style and comment improvements and fixing (almost) all the issues reviewed and reported by mcrittenden (#165) and cweagans (#168).

Jen's branch is unfortunately missing some important changes (non CSS/HTML stuff) due to some problems with Github itself. But I've merged everuthing into my branch here: http://github.com/dickolsson/Bartik . I've tested my branch and everything that is implemented seems to work properly.

I know Jen is planning to roll a CVS patch later today. But I don't know from which branch she wants to do that. So I am leaving that for her to decide.

Here is a new roll at this. This patch mirrors my branch at github (http://github.com/dickolsson/Bartik) which includes:

• All the latest CSS and HTML stuff from jensimmons
• Dmitris theme settings stuff and security fixes
• Fixes and cleanups from mcrittenden and cweagans reviews
• Cweagans fix to #link-wrapper
• And some other code style and comment cleanup by my self

The patch does not include:

• Necessary images (grab them from #144).
• preview.css needed for the color module.
• Unnecessary template files that was included before. Although jensimmons still wan't them there for new themers. So we should open up another issue about this!
• HTML coding style issues mcrittenden had in his last review. This belongs in another issue as that issue also applies to all other template files in core.

So, still some work left to do. Although, it's ready enough for another round of reviews!

+++ themes/bartik/bartik.info	31 Jan 2010 23:30:23 -0000
@@ -0,0 +1,31 @@
+description = A flexible theme with many regions. Suitable for hacking to make your own theme.

For a lot of people 'hacking' means breaking into people's bank accounts... and I'm sure that includes people who get as far as installing Drupal and changing their theme. Let's use non-jargon here, eg 'modifying'.

+++ themes/bartik/bartik.info	31 Jan 2010 23:30:23 -0000
@@ -0,0 +1,31 @@
+regions[sidebar_first] = Sidebar

Only one sidebar now? How easy is it to modify the theme to put it on the left instead of the right? If we're aiming to be easily modifiable, changing sidebars from side to side is something people will want to do.

+++ themes/bartik/comment-wrapper.tpl.php	31 Jan 2010 23:30:23 -0000
@@ -0,0 +1,48 @@
+    <?php if ($node->type != 'forum'): ?>
+      <h2 class="comments"><?php print t('Comments'); ?></h2>
+    <?php endif; ?>
...
+    <?php if ($content['comment_form']): ?>

Testing for node types that might or might not exist feels a little hackish... I guess there's no way round this though :/

+++ themes/bartik/comment.tpl.php	31 Jan 2010 23:30:24 -0000
@@ -0,0 +1,103 @@
+        <?php print t('!datetime.', array('!datetime' => $created)); ?>

Full stop here? Not sure about this; I'll have to check it on actual output, just looking at code right now.
Does a node's $submitted get a full stop? For that matter, how come node.tpl gets a ready-made $submitted and comment.tpl has to build it itself?

+++ themes/bartik/node.tpl.php	31 Jan 2010 23:30:24 -0000
@@ -0,0 +1,131 @@
+      // We hide the comments and links now so that we can render them later.
+      hide($content['comments']);
+      hide($content['links']);

I've not played around with D7 theming yet to know whether this could be moved to template.php...

Powered by Dreditor.

@joachim - I'm pretty sure we have comment-TYPE.tpl.php suggestions, not sure about comment-wrapper-TYPE.tpl.php, but if there's not, then it's better to support core modules visually than not.

Agreed that 'modifying' is better than 'hacking', could we get away with "copying and modifying'?

orry if you felt you need to defend the work you did on IRC. Indeed IRC can be a bit of intimidating -- maybe you should work off IRC a bit? If you need help with PHP/CVS/whatnot I will be happy to lend a hand nonetheless.

Well, the deadline for this patch was set to...um. Today. So, since you were unavailable (at least when I was around), dixon_ and I did some work on the theme today. This is something that a lot of people really want to see get into core, and the changes were not stylistic changes. The theme should still look/perform exactly as you intended. I don't know about all the changes that dixon_ rolled into the patch, but I know that the work that I did was simply code cleanup, that would get rid of reasons to not put this in core (and would have been necessary anyways).

The trick here is trying to balance your vision with the Drupal core development workflow and the Drupal coding standards. For example, having a copy of the core template files for no reason is not something that really happens. If there's no change to the template, there's no reason to include it in your theme. It's just extra code that will need to be maintained.

So, more to the point: what do you want to have happen with this theme? You've got at least 10 people that have previously done work on the theme (according to the Github network panel thing), and I know that I'm ready to put in a few hours to get this thing ready to go. You make a list, and I'll start working through it because I want to see this in core. It would be a shame to ship with only Garland, Seven, and Stark.

er, whoops.

I really really _really_ don't think this can go in with those extra templates in there. There's no reason to have them. It's just extra files to maintain for the next couple of years. The README should be in drupal.org's doc section.

We don't have any written, defined code standards for CSS comments, but I think Garland, as a theme that's been in core since 4.x, serves as a good example of what to do/what not to do.

Will review when there's another patch.

I think having a readme is a very good idea (a hacker will first have a look at the files present before considering googling for "the right way").

However, instead of proposing to people in there to duplicate the theme I think creating a subtheme should be instructed.

Also this may not be in the scope of adding this theme but what about adding to the info file the details of a non existing style sheet? a local.css file where if a user wants to change something in the theme, the user is instructed to create a local.css which will come last in the list of css files.

Can we get a summary of exactly what was incorporated into the latest patch, as well as a new patch to review? And is the demo site updated accordingly?

--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ themes/bartik/README.txt	31 Jan 2010 13:47:49 -0000

I've said it previously, but I still really disagree with a README being included with a core theme. Why should we include user manual information along with the code, when there is already a centralized manual on d.o?

+++ themes/bartik/comment.tpl.php	1 Feb 2010 15:17:43 -0000
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ themes/bartik/html.tpl.php	30 Jan 2010 23:27:10 -0000

As I previously said, this should not be included. It's duplicating (verbatim) code that's already found in core.

+++ themes/bartik/html.tpl.php	30 Jan 2010 23:27:10 -0000
@@ -0,0 +1,57 @@
+// Last conformed to D7 Head - v 1.3 2009/11/16 05:34:24 webchick

If this file is going to be included, this line needs to be removed, along with the one directly above it (empty line).

+++ themes/bartik/html.tpl.php	30 Jan 2010 23:27:10 -0000
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ themes/bartik/maintenance-page.tpl.php	30 Jan 2010 23:27:10 -0000

+++ themes/bartik/maintenance-page.tpl.php	30 Jan 2010 23:27:10 -0000
+++ themes/bartik/maintenance-page.tpl.php	30 Jan 2010 23:27:10 -0000
@@ -0,0 +1,97 @@

@@ -0,0 +1,97 @@
+<?php
+// $Id$
+
+// Last conformed to D7 Head - v 1.8 2009/08/03 03:04:33 webchick

Ditto.

+++ themes/bartik/node.tpl.php	1 Feb 2010 15:23:35 -0000
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ themes/bartik/page.tpl.php	1 Feb 2010 15:27:01 -0000

Not sure how I feel about two div tags being on the same line throughout this file. It just seems a little messy/hard to match with the closing tag. Not a huge issue, though.

+++ themes/bartik/css/style.css	1 Feb 2010 18:55:13 -0000
@@ -0,0 +1,705 @@
+/* ---------- Overall Specifications ---------- */

In the other CSS files, your comments are one way, and in this one, they are another. Please pick one :)

Additionally, a bunch of git files got into this patch (the entire .git directory for the theme). Those will need to be removed.

Powered by Dreditor.

Oh, looks like you got the git files taken care of. Disregard :)

"b) A theme setting so people can easily change the "published" by text. (thanks dmitrig)"

This is puzzling. Why? I would remove this, since no other theme offers this and it feels like fluff.

Oh. Everything else sounds good though. Let's get some more reviews. :)

Good afternoon,

Took a very quick look through the comments. Is anyone currently performing an accessibility review of this theme? I would offer to do some accessibility reviewing on this theme if it is reasonably foreseeable that it is going to be committed to d7 core.

Of all the themes being considered for core, this one definitely has the best shot. It's been developed the longest, and has the most people involved in getting it ready.

I would love to see you do an accessibility review on it, Everett!

@webchick, @jensimmons

Can someone point me to a demonstration site with a reasonably recent version of the theme? I can install head / the theme myself if necessary, but ideally I'd rather not :)

Great work everyone, there is good progress here !

I have one little issue though.

+        </div> <!-- /name-and-slogan -->
+      </div> <!-- /logo-title -->

I do not find it useful to add a comment to enclosing tags everywhere. I do not think it really adds any useful information for developers / designers (not enough IMHO) and in the end this is in the output, which means bigger files too. I know it is done in Garland but I disagree...

Edit : Moreover, in Garland, this is done for hard to read nested markup in the main div and the header in the page template, not anywhere else.

Everett,

http://bartik.milkweedmediadesign.com/ is where Jen's been updating the theme. It's at worst 5 or 6 hours behind at this point.

Steven, that's a Garland site right now... can someone with admin rights switch the theme please? :-)

Changed it back to Bartik: http://bartik.milkweedmediadesign.com/

A very preliminary accessibility eval of the Home page from perspective of a blind user with FF3/JAWS10 (screen-reader).

I'd say that this is all pretty good. The general comment that I would make is ordering of information in the DOM. The User Login and Search blocks, along with the footer are all in unpredictable positions. From the point of view of a non-mouse user who needs to tab through the screen it might be nice to place the login and search nearer to the top of the DOM. "Skip to Login Form" and "Skip to Site Search Form" links might also be beneficial. I would also recommend checking to make sure that flow of focused elements follows logical visual page flow.

1. Logo: the title of the anchor and the alt of the image are both set to "Home". This may be the node-title, but is not an accurate description of the graphic.

2. No heading before the main menu, other than the site title. This is not necessary, but would be consistent with accessibility improvements that were made to Seven. May be more necessary as modules are enabled and if a sub-theme is derived that makes structural changes.

3. The list of tags for each node listed on the home page are not structured as a list.

4. The "Read More" and "N Comments" links are structured as lists, even though there is only 1 item in each list. This may be appropriate if modules are likely to add to these lists.

5. "Some More Text" is confusing, but I imagine is just for testing.

6. "Who's New". My first question was new at what?

7. The footer is not structurally where I would expect a footer, which is at the end of the document.

8. Recent Content is structured in a table with two empty columns. Is there a reason to use a table for this data structure?

9. Ideally the search form would have a label. The title attribute on the input field is helpful to screen-reader users, however a label provides a larger clickable target for mouse users who have difficulty with fine mouse movements. This obviously depends on the size of the input field, which I cannot assess.

I'll field a few of these....

> 3. The list of tags for each node listed on the home page are not structured as a list.

They ought to be. They are in Garland.

> 4. The "Read More" and "N Comments" links are structured as lists, even though there is only 1 item in each list. This may be appropriate if modules are likely to add to these lists.

Yup, node links get added to by lots of modules. So this bit's fine.

> 6. "Who's New". My first question was new at what?

That's straight from core. It's who's new on the site.

What I didn't mention in my accessibility review is that some of the problems may be caused by core itself. No reason not to fix theme in the theme if it is reasonable to do so.

@Everett: the order of blocks like search and login can be managed by the site admin, so I think the theme does not have to worry about that.

I have been trying to tab through the site myself, but I noticed that there is no tab focus border around links in both IE8 and FF3.6. This is caused in reset.css:

:focus {
  outline-color: -moz-use-text-color;
  outline-style: none;
  outline-width: 0;
}

(This is exactly the reason I'm not a fan of reset.css, it resets too much IMHO.) I have always been told that the focus border should not be removed without providing alternative :focus styling. Accessibility experts, is this indeed a problem?

@marcvangend

Thanks for pointing this out. If a non-mouse user cannot tell which element currently has focus then it is a huge problem for accessibility. Imagine not having use of the mouse, if you don't know by tabbing which link has focus then the site is virtually useless.

I've been giving some thought to how to make the order of content in the DOM a little less confusing to a screen-reader user. Screen-reader users navigate through the DOM not the visual appearance of the page.

I think I would say that there could potentially be two heading level 1 elements on the page.

1. On the homepage the first h1 will be the site title and the second h1 will be (perhaps using .element-invisible from system.css) a heading that indicates that this is the side bar, or whatever language seems appropriate to describe the block content that comes after it. Keeping in mind to use plain language for the text in the heading and not Drupalese.

2. On any page where a single node is displayed as the primary content the node title should be the h1, not the site title, or perhaps both. I think we ended up with agreement on this with garland, not sure if it ever got implemented. The reason that we need a second h1 for this theme is because it is content-first. Content coming before other supplementary content can make it more difficult to find / reach the supplementary content for a screen-reader user.

As I suggested above for login and search, a "skip to ..." link might also be useful "..." being replaced with the wording from the second h1 heading.

And again, ideally the footer would be actually at the foot of both the visual page and the DOM.

I think this would provide some accessibility improvement and would not be visually intrusive.

One question on the skip links: are they styled to appear on focus/active/hover? This is useful for non-mouse, non-screen-reader users; and I believe this is the default in other core themes.

@ #206:

Another thing I experienced with the default (dotted) outlines in Firefox is that it can slow down any running JavaScripts terribly.

Perhaps a light gray background is an acceptable custom style for a:focus (and a:active)? And the default link color as border color for input:focus, select:focus, textarea:focus…

It can be any style, really. As long as it's recognizable as the currently focussed link/field/whatever. It should not be confused for a.active, for example.

@#208

That is interesting Firefox behaviour that you have described. I'd love to know if you've noticed this mentioned by others on the web.

As far as colours go, I'd recommend using a tool like http://www.snook.ca/technical/colour_contrast/colour.html to make sure that all colours used in the theme (including any suggestions for focus etc. are WCAG compliant at the AA level. This basically means ensuring that the contrast between foreground and background colours is sufficient to make the text clear to low vision users. This may already be part of the Color module.

@#209

I've only seen it on OS X so far, not sure about Windows/Linux versions. I haven't found anything about it elsewhere yet. It's probably not something we should care about with this theme, I believe it occured only when scripts were used for animations, e.g. a slideshow.

+<div id="comments-wrapper">
+  <div id="comments" class="<?php print $classes; ?>"<?php print $attributes; ?>>

Can these two be combined?

Also,

+#comment-wrapper {
+  border-top: 1px solid #d3d7d9;
+  padding-top: 15px;
+}
+#comment-wrapper h2.comments {
+  margin-bottom: 1em;
+}
+.node-teaser #comment-wrapper {
+  display: none;
+}
+#comment-wrapper  div.user-picture img {
+  margin-left: 0;
+}

I don't think these are working since it's supposed to be #comments-wrapper (note the 's').

@jensimmons I generally style :hover and :focus together, so the tabbed navigation highlight gets the same style as the :hover.

The reason, btw, that I want these little one-off doo-dad gadgets to be removed, like the "Published on" string being configurable in the UI, is because it makes it incredibly confusing for end users who get used to the way one of the core themes works, then go to do that same thing on another one, and then can't find that neat little thing they played with before. It also causes maintenance overhead because changes to themes can't be made consistently throughout.

If such features are found to be useful, they should be added to core itself (like Color module) so that any and all themes can hook into them. And since Drupal 7 is closed for new features, this would have to happen in Drupal 8.

So please keep these themes to just that: themes.

Moreover, text strings can be overridden using locale, so that is easy to change.

The module for that: http://drupal.org/project/submitted_by :)

It's definitely one of the top 3 modules I've linked to people in #drupal-support

I'm not sure about the lack of fieldset on the theme settings -- but Garland is guilty of this too, so I would say leave that till this theme is in, and then discuss and fix across all themes in a new issue.

There's however a layout bug with content creation -- this is the create article form:

And some overflowing in the vertical tabs...

And another one: menu admin. Presumably other admin pages that list a name + a description too?

I really don't like $secondary_menu being in the footer. Given the feature where you use it to show the 2nd level of the Primary menu, it's just weird.

Been playing around with Bartik as downloaded from Github today using FF3 and IE6. It's great, very impressive and a nice demo of what Drupal 7 can do that Drupal couldn't before. Comment display is particularly nice.

I'm not qualified to review theme code, but from clicking around (including playing with views) I didn't notice any bug not already filed.

I added one new issue:
#713444: Add a second sidebar
which I think is important, because if Bartik is going to claim the core default spot, I think a single sidebar always on the right is too prescriptive.

It would be great to see a status update on this thread.

Downloaded Bartik and played with it a bit.
Nice work indeed :)

What can use a lot of work is color module support. At the moment only the header and some textst are recolored, but a lot of areas remain untouched. A good point for recoloring are IMHO Headings, which are hardly touched. (see attached image. Also the Footer area and the gray area below he header and above the footer remain grey.

Am no color module specialist, but I guess the current restrictions of color module do not help with flexible recoloring. Can one define certain Elements like .node h2 or whatever to be recolored?

one more image

@eigentor: there is only so much that can be done with color module right now. We are dependent on #693504: Color.module does not support more than 5 colors and has hard-coded labels getting in before we can move forward on that particular feature.

I am working on getting two sidebars in, but the current widths of the content + sidebar do not properly add up.

content			680
	Margin-left	15	
	width	640	
	Padding-right	20	
	Margin-right	5	
			
Sidebar-first			265
		25	
		225	
		15	
			
TOTAL	945

There is a spare 15px somewhere!

I've added support for two sidebars: http://github.com/joachim-n/Bartik/commit/13180dbfe12bda2f6aa3396a22bfb6...

Note that the missing 15px are still in there! I am not sure what the underlying column grid is meant to be, so my attempt at fixing this would be hamfisted.

BTW, a mention of the underlying grid and its dimensions would be a nice addition to the README file.

The other big thing to look at is the placement of the secondary menu, but that really need design input from jensimmons.

I think all core themes should support the core feature of coupling the primary and secondary menu output to the same actual menu, which to my mind means showing them reasonably close to each other in the theme, so the connection is apparent to the user.

Didn't know Stark did that. Confirming it does. Stark is wrong -- or at least doing it in a way that is so basic it should be ignored.

@joachim

I agree that coupling the primary and secondary menus is desirable, and in markup along with visual placement.

1. If there is a logical relationship between the two menus then it makes sense for them to be placed in visual proximity.

2. As part of a semantically structured document, it makes sense to group related content in structural proximity.

3. Semantically structured documents are a good place to start in ensuring documents are accessible to persons with disabilities.

I second what Everett has said. If it is possible to solve this placement issue in a way that links the menus semantically, that's the way to go. That approach follows universal design principles and will lead us to better results in the long run.

Everett, I like what First Baptist Church of Wichita has done on their site, except that it isn't accessible (more on that below). Their primary navigation has seven terms:

• Home
• About
• Grow
• Community
• Ministries
• Resources and
• Contact

Each of these, except for Home and Contact, reveals a dynamic secondary menu on hover, but not on focus. In text-only view, when the secondary links do appear, they are always in the same location — as a new list after the list of primary navigation links. So I'm guessing that people navigating without a mouse would never know that the secondary menus are there.

If there were a way to place each set of secondary items as a subordinate list following its respective primary item and to activate revelation of the secondary menu on focus, I think this would be accessible. Jen and Joachim, FBC-Wichita has a pretty talented team working on their site. Let me know if this interests you and I will ping them to try and get their interest in adding that feature to this theme.

Cliff

@cliff

There are benefits and drawbacks to any approach. The drawback of the approach that you have mentioned above can be illustrated with an example.

A site has 7 items in it's primary menu.
5 of these items have 5 sub-menu items.
There are 32 menu items.

A keyboard user wants to get to the 28th menu item and must tab through 27 items to get there.

I haven't really done much research on fly-out / pop-up menus and can't really suggest the common accessible option at this time. Perhaps what you have suggested is actually the best of all approaches.

> If there were a way to place each set of secondary items as a subordinate list following its respective primary item and to activate revelation of the secondary menu on focus, I think this would be accessible.

No, that's going way too far, and it's also not at all what this is about.

If you want a submenu as a child list of the menu, then use a Menu Block. Those do all that already, and there are lots of modules out there for turning them into fancy JS popups. This is about the two lists of links you get built into the theme layer.

So, to explain more.

The Drupal theme layer, as far back as D5 and possibly before, supports the output of two lists of links in your theme. In the theme settings admin they are called 'Primary links' and 'Secondary links'; but they are commonly just called menus, because the links come from the menu system. They are not menus because they only show ONE level of links.

Very confusingly, the menus are ALSO called 'Primary links' and 'Secondary links', but the two are not *necessarily* connected. You get to pick WHICH menu supplies its top-level links to the two theme link lists.

So you could have:
- Primary links: top-level links from Primary menu
- Secondary links: top-level links from some admin-created menu

Where this gets cunning however, is what happens if you choose the SAME menu as the source for both links. In this case, they magically couple together, and the Secondary links shows you the 2nd level from the menu for the page you are on.

In this illustration I am on page Beta, and the Secondary links shows the items One to Four because those are the child links of where I am.

So -- to recap, this is a cool feature, and the two menus should be in reasonable proximity to one another so that they make sense when it is in use.

(As far as I know, there is VERY LITTLE in our Getting Started docs about this, so if anyone feels like taking the picture and my writeup and integrating it in, that would be great!)

I opened an issue about the problem with the primary and secondary links implementation
#698014: Theme settings for main/secondary variables mismatch with menu settings
and I posted a patch to convert them to standard blocks and support two level navigation
#503810: Convert primary and secondary menus to blocks

If you want to have a look, maybe with can solve this in Drupal core.

I think it's time for another round of interim patches, to see if we can get this wrapped up by March 1.

I don't think we should count on #503810: Convert primary and secondary menus to blocks happening -- it's too big a change and there are too many unresolved complications around it.

I had added comments relative to bartik in http://drupal.org/node/693504#comment-2613184 and thought I should repeat them here.
I had downloaded the bartik theme on 2/17 from Joachim's git hub and installed on ver 7 alpha 1 on a Mac. I had also applied the 693504-color_module-custom_fields-35.patch. I would get a blank dialog when I tried to look at the setting for bartik. Taking a clue from the error messages I looked at the color.inc files for bartik and garland. It looks like the array structure for 'schemes' and 'gradient' had changed. I made corresponding changes in bartik's color.inc to match the style of garland and it appears to work.
Again I am new to drupal and am not sure if I messed something else up. I keep getting a missing button.png message in the webservers logs as well as something called 'system' in drupal's root. Should I have gotten my copy of bartik from a different github? By nature, I prefer to work with the latest.

In file style.css line 493 looks like '../' was dropped from url, should be
background: #161617 url(../images/footer-background.jpg) repeat-x top center;

I think it's time for another round of interim patches, to see if we can get this wrapped up by March 1.

As far as I can see from the Bartik issue queue: http://drupal.org/project/issues/search/drupal?issue_tags=Bartik
and github: http://wiki.github.com/jensimmons/Bartik/
and this thread
nothing's happened here for at least a week.

That's a real shame. I don't think we should lose sight of the initial reactions of Dries ("Nice!") and webchick ("Wow!!") in among all the details. Personally, I think this would be a fantastic, and important, addition to D7.

With a week to go before Mar 1, what can we do to make that happen? For all the daunting thread length, I'm not aware of anything like a critical bug in this code, so I suggest it ought to be committed and worked on normally. I think it's fantastic what jensimmons has done, and I am not sure it's reasonable to expect one person to have to field all the rest of it.

So here's a patch.

I hate CVS and am only learning git, so the patch may not be what's required, and only relates to joachim's latest commit at: http://github.com/joachim-n/Bartik/commit/13180dbfe12bda2f6aa3396a22bfb6.... It excludes a few more recent minor changes from jacine as I don't have the time to figure out how to merge the two, but I'm sure that can be sorted out.

Let's see what the bot says.

I seriously think we should create a project page for Bartik and move those issues over there instead. Who knows, we might want a Drupal 6 branch open for it eventually! I'm not saying this is not core material, I'm just saying it might be easier to handle and know what's going on.

Here are my notes after applying the patch and trying it out. Forgive me if I'm bringing up things from past comments as I haven't read through 250 comments :-) .....

1. Looks really nice!
2. Looking at README.txt, it says "Duplicate the Bartik directory (the folder)". Instead of encouraging people to fork Bartik, shouldn't we encourage people to run Bartik as a sub-theme? The README.txt formatting should match Stark's?
3. I probably have an old CSS file, because I'm getting the attached screenshot after applying the patch and adding the images/css...
4. Might just be me, but I'm not seeing the Color module settings in the Bartik settings page.
5. I like the idea of the sample blocks. Should these instead be pushed into Drupal core?
6. We probably don't need "Last conformed to D7 Head - v 1.3 2009/11/16 05:34:24 webchick" in there ;-) .

It's looking amazing! Could we open up a project page for this so that it's easier to see what's going on with it, as well as help clean up the Drupal issue queue? THANKS!

YAY!!

Thank you, Jen. It's going to be so much easier for people to help on this when there is one canonical place to go to find the latest copy of the code.

Also big thanks to willmoy for pushing this along. :)

One canonical place for issues that would prevent bartik being released in core:
http://drupal.org/project/issues/bartik?text=&status=Open&priorities=1&c...
I've removed the bartik tag from its issues now, per 'Do NOT use tags for duplicating the "Assigned" or "Component" fields.'

There are only six critical issues: two have code already thanks to jacine and joachim; two seem very simple; core by March is not impossible. There are only six normal issues too.

It's going to be so much easier for people to help on this when there is one canonical place to go to find the latest copy of the code. —webchick

If anybody who understands the code could pull together the strands at http://github.com/jensimmons/Bartik/network to give a single code base to test and patch against so that jensimmons can commit it to CVS under the bartik project that would be just lovely.

> If anybody who understands the code could pull together the strands at http://github.com/jensimmons/Bartik/network to give a single code base

Surely only jensimmons can merge into her branch? Anyone else can make a branch and pull everything in but how would we know which one?

So. Here's where I'm at.

I would really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really like to see this in core.

But this issue is incredibly heart-breaking because there are literally dozens of people who are ready, willing, and able to help with this, and they are currently totally blocked from doing so. And they have been blocked since February 5th, when the last "official" patch was posted. There are at least 7 versions of this theme floating around, but no one knows where the most up to date version is, and no one in their right mind is going to sit down and dedicate 2+ hours to review the theme and fix bugs (which is something we desperately need a lot of people doing, if this is to become "core-worthy") if they aren't absolutely 100% sure that the copy of the theme they're doing it against is the "real" one. This ambiguity created by GitHub forks and the like is literally killing this initiative, which of all the contenders definitely is the furthest along, has the most community interest and involvement (including an accessibility review!), and thus has the best chance to get into core.

Jen, I know a big part of this is that you've been going through some pretty heavy stuff lately, and I most certainly do not want to add to your stress level. Tell me how I can help. Do you simply need more time to get some stuff cleared off your plate? Is there someone you trust to delegate the leading of this theme to in your place, who has more time available? Would you be willing to bless a team of folks to go off and do stuff in CVS, with you coming in periodically for reviews to make sure we're not going against your wishes design-wise? I'm definitely open to ideas. What can we do to keep this moving?