Fix taxonomy term displays

Removing float:left from the class "field-items" makes all items appear inline with the Label. See the attached patch.

Hoople, your patch is from a git repo, could submit one for Bartik checked out of CVS HEAD.

I rerolled the patch ... and it seems to work well:

This is not what I am seeing in Firefox3 (Win7) and IE8, IE7 is very broken:

I believe this was fixed in the push to get Bartik ready for core.

Nope, this was not fixed - fresh screen-shot of the issue in IE7 in Alpaha6 today.

Moving to right queue.

Nobody likes IE7 ... here is the screenshot from #8:

This patch fixes the IE7 issue...

With a definite time crunch coming I think we just need to move ahead and fix all the issues with terms - there at least one other issue not being reported - the use of the field name class rather than sticking to the field-type class to deliver styles to all taxonomy term fields.

Additionally there are a couple of other issues floating around that deal with the display of taxonomy terms:

#876940: field-label-above has field labels in-line and floating left
#845722: Term reference fields have no linebreak
#847632: Body floats onto term field when placed below them.
#833038: Make taxonomy styling apply everywhere

All these issues essentially revolve around the positioning of terms - its generic issue with flow on ramifications. Lets fix it once.

Attaching a starting patch that:

1 - removes the use of field name class, only uses the field type class
2 - does not float the label - we shouldn't be doing this because core can do this with display settings
3 - keeps the terms inline, even if the label is above or hidden

Keeping as normal but really this could be major, since it a) breaks the display in several ways and b) overrides a core display setting so users are left with a wtf when they try to move the label above.

The patch attempts to solve (b) but needs further work and testing for (a).

OK, here another patch that should solve 12-a and 12-b. Adds overflow hidden to the field wrapper.

#13: bartik-fix-terms-display-819996_v2.patch queued for re-testing.

Ugh, reroll. From style.css version 1.5 to 1.22, there were quite a few changes made. Some things even seem to have been changed already (e.g., div.field-name-taxonomy-tag to div.field-type-taxonomy-term-reference).

I think I got everything, forgive me if I didn't.

#15: drupal-bartik-fix_terms_display-819996-15.patch queued for re-testing.

patch fixes problem with IE6 and IE7
RTBC ?

Jeff, since you were the grand architect of #660614: Remove #block-system-main dependency, fix font sizes, remove crufty CSS, and that's mainly what forced the reroll, I'd love for you to ensure that I got the intention of this patch correct. Visually and code-wise I'd say it's RTBC.

The main thing is to test with every possible display setting, for both Article content type and Forum content type (the taxonomy fields are in different positions) and even adding some new taxonomy fields and placing them in various positions in relation to other field types. D7 is so different with this whole fields in core we have to account for many more permutations than ye olde D6 $terms.

Any entity that can have a taxonomy term field needs testing - so its quite a lot of testing we need done. I will post some screenshots shortly of what its meant to look like for each display setting.

OK, it pretty late here so I am just posting a basic screenshot showing the default display settings for the label.

One of the big thing to look out for is how other fields react around them, such as an image field with the label showing and so on - is the spacing between fields correct and look good (is there enough white space).

Refer to the screenshot in #10 for the big problem with IE7.

All these were taken in Firefox 3.6.x on Windows 7, with the patch in #15 applied:

inline label does not happen when "terms width" + "label width" > ".field-label-inline" width (case of too many terms)

see screenshots

some RTL tests more

@21 - not following what you refer to or mean by inline label does not happen when "terms width" + "label width" > ".field-label-inline" width (case of too many terms). Is your report/screenshots with the patch or without the patch?

I found strange issue with Firefox 3.6 in RTL when two taxonomy term fields - the top one is display label Above, the bottom on display label Inline (In IE7 this is playing OK).

Rants... but oh so valid...
#784792: Field Type Template Suggestions
http://drupal.org/node/784792#comment-3550660

@jeff
when setting label "inline" and have too many terms
it looks as if label was set to "above"

its easier to understand if you see
http://drupal.org/files/issues/819996-bartik-term-displays-d7_2.jpg
second picture "Firefox Inline label"

@25, no need to reiterate your posts, we can see them. You need to say what operating system you are using and if they are before or after screenshots.

@26 I was just answering your comment from #23 !!!!

windows xp - after screenshots

Looks like #784792: Field Type Template Suggestions is gaining momentum and could get in - we should consider this blocked and wait for an outcome of that issue - because if it gets in we'll need to consider where to add the new function or template for taxonomy reference fields - whether this will happen in the theme or somewhere else. That said, if it does get in we can pretty much assume traditional list markup will replace the DIV's and can start back on the CSS at least.

Hooray #784792: Field Type Template Suggestions just landed!

So, we need to decided if we're going to add a template to Bartik to override term reference fields and make them lists again.

Sounds like a great opportunity to use that new feature (and demonstrate how it works).

Demoting back to normal, since no reason was stated for why this bug is major. Overall, this issue sounds and looks annoying, but it's limited to Bartik and affects a few users only.

Here's a patch with my take on this issue.

Attached are screenshots for FF, IE 6/7/8. I also checked Chrome/Opera and they're fine.

P.S. If this gets in, http://drupal.org/node/759734#comment-3761914 is no longer needed :)

Oops, forgot to change the header for the new template file.

Sorry, git is playing tricks on me.

+++ themes/bartik/css/style-rtl.css
@@ -88,12 +88,18 @@ ul.tips {
+  float: right; /* RTL */
+  padding-left: 5px; /* RTL */

No need for the /* RTL */

+++ themes/bartik/css/style-rtl.css
@@ -88,12 +88,18 @@ ul.tips {
+  padding: 0 0 0 1em; /* RTL */
+  float: right; /* RTL */

Same thing here.

Functionally this looks good to me.

Powered by Dreditor.

You're right, thanks.

+++ themes/bartik/templates/field--taxonomy_term_reference.tpl.php
@@ -0,0 +1,52 @@
+<div class="<?php print $classes; ?> clearfix"<?php print $attributes; ?>>

This will result in clearfix class being added twice when the field label display is inline.

Do we really need the template, it would be more performant if we used a function instead.

Powered by Dreditor.

New patch with the approach suggested in #37.

I tested this on FF, Chrome, IE 6/7/8 (LTR & RTL) and they look just like the screenshots in #32.

Ok, I have reviewed the patch in #39 in some depth and to be frank I am not entirely convinced with the approach taken in the CSS and there is an issue with the use of $vars in the function, should be $variables to be consistent with other core functions.

I think we can utilize cores "inline" class the way it was intended it only add it when the label display is inline and leverage cores field label floats also - this cuts down the CSS a lot and there's no need to rely on chained selectors. This is how the inline class was meant to be used - for when we want a block level element to be inline, its really a perfect fit, but do not use it when we want the block level element to be block level.

I also think the margin should be the same as for paragraphs, so it has margin bottom 1.2em instead of margin top 1em - Jen might like to chime in here.

Heres a patch to show what I am thinking. I'm not adding any screenshots because it looks the same as #39 basically, just different approach in code.

Both of these patches are on the right track, I think Jeff's is a little cleaner.
#40 is RTBC for me, hopefully Jen can give a yay or nay on this today.

Well, then let's clean it a little more :)

I don't think that we should set $element in the theme override function if we only use it once, and also changed the syntax a bit to make it shorter. I saw in Seven's template.php that it's ok to do it this way (seven_admin_block_content).

Also, there's a problem with font sizes in list mode (frontpage) vs. node page in Chome and IE 6,7,8. Attached is a screenshot from Chrome. This problem is also present in my patch so let's see what Jen has to say about this.

the screenshots from #42:

FRONT

NODE

Also seems weird that "Tags:" is inline on "node" but not "front" and "Other tags:" is inline on "front" but not "node" ... Shouldn't they all be one or the other? (I vote never inline, but I dont care that much as long as they are consistant)

@bleen18: I have different display settings for default/teaser because i wanted to test all possible situations.

Patch in #42 is looking good, nice improvement there, I did a quick reroll setting a specific font-size for both teaser and full node view. For the teaser I locked it in at 11.5px (0.821em) and for full node at 12px (0.8em), assuming browser default of 16px.

What do you guys think, either we could make them the same font-size (same for both teaser and full view) or allow them to be different like .node .content?

Good question. When I saw those screenshots I thought it was a mistake, or that one was scaled, or something.. and in fact pulled them into Photoshop to compare. But once I realized the different sizing was intentional I thought it was ok. I think in a full river of news, the smaller tags will make even more sense.

Looks like an extra file snuck into the last patch:

Index: .project

Ops, forgot to exclude the Eclipse project file...

This looks great. Great use of #784792: Field Type Template Suggestions. RTBC.
Double-space before a /* LTR */, not sure if that matters, so I rerolled.

"Setup environment: failed to drop checkout database."

#49: drupal-bartik-819996-49.patch queued for re-testing.

Committed to HEAD.

Thanks for all the awesome testing work on this, folks!!

The patch #49 overrides the default theme_field() but does not include the item_attributes variable, causing the RDFa markup of these taxonomy terms to have disappeared in bartik. This patch restablishes the missing $variables['item_attributes'] in bartik_field__taxonomy_term_reference().

Adding some test to rdf.module so this does not happen again.

Applying just the test, it fails. Good catch.

Committed to CVS HEAD. Thanks.

Followup : #1106344: Taxonomy term reference field headers always should be rendered with a HTML header (invisible or not)
Feedback welcome from folks from this thread.

I have the problem with Drupal 7.9...

If I set "hidden" or "inline" for the label of the field in the display management of the vocabulary, the label is still displayed above the content !

NB : I don't use the bartik theme, I use the Zen theme (3.1).

Please open an issue in the Zen issue queue: http://drupal.org/project/issues/search/zen