No styling for sticky

I think this simple border and background color should do the trick.

Maybe this works better. But what's up with the rgba notation for the color? All other colors are defined as hex.

Not sure about that bg-color, it seems not-quite-right with most color schemes.
Sorry, no suggestions for now, just a quick review and a (hopefully) working patch to build on.

RBGa is awesome. It's all over Bartik. It makes translucence colors. That way instead of a exactly-grey background, you get a grey that's slightly tinted with the page background color — a little blue, or a little yellow. When you change the background color of the page, the background of the object with translucent color alters slightly to match.

But in order to use RBBa, you have to provide a fallback for IE, and neither of these patches have that.

Rerolled patch #4 to include IE hex color fall backs. I added a style sheet to target IE8 and below so the one line in ie8.css covers IE6-8. I did it this way initially because I saw that's the way the IE7 style sheet was added. Let me know if you have any other ideas on doing this re: Giving each IE version it's own style sheet instead of one covering all three.

Ignore that last patch/comment, not sure what I was thinking on that one. Here's a better patch.

Looks good to me... but could probably use slightly-rounded borders, like the sidebar blocks (probably 4px, just the same). I would make the patch, but I realized my new work Mac doesn't have CVS on it (yet).

@geerlingguy Maybe, eitherway not required for this issue to be committed. Looking at the comments, they don't have rounded corners either I would say its better to stay consistent with that? We can follow up if jensimmons feels strongly about going with rounded corners.

Sounds good; didn't think about the comments, so that's a good point. Perhaps, though, we should wait for jen to give the rtbc?

@geerlingguy thats fine, this is a small issue.

Just tweaking coding style to keep in line with #889256: Clean up CSS coding style.

Just a minor thing; Mark Boulton explained today that a border around a box is unneeded if it already has a background. A background alone would do to group items. That said: it looks great!

We need to do more extensive testing before marking stuff as RTBC and please note that color changes need accessibility reviews, we're getting too many patches marked RTBC too quickly and causing regressions etc.

Several issues with this:

1) This proposed colors fail contrast requirements - see screen-shot - the link color doesn't work so well with this background color.

2) Nowhere in Bartik are the other fall back colors defined in specific IE stylesheets - other browsers might not support rgba either therefor fall back color is included in style.css - e.g.:

#navigation ul.links li a:active {
  background: #b3b3b3;
  background: rgba(255, 255, 255, 1);
}

I tend to agree the border may detract from the content - thinking Jen needs to look at this and give some direction/design insights.

Do we really need to style the full node view? You can see below how this causes a problem with comments...

I know previous themes have, but, the publishing option is "Sticky at top of lists" - that says to me we want to highlight the node or nodes as being more important by elevating them to the top of a list - so maybe we want to style them in some way to reinforce this importance. However, why would this imply we want to style the full node view as well?

Naah, just the teaser indeed. Sticky is a contextual thing, only relevant in relation to other items, no need to style the singe full node view.

Ouch, this contrast thing might open a can of worms - I was under the impression colors had been tested and passed in Bartik but I'm getting failures for the link color on a white background. I know blue is hard to work with.

@jeff burnz I am marking it RTBC, because we are meaning to release within a few weeks - I cant guess from missing reviews that stuff isn't correct.

I was under the impression, the border was the primary focus here for accessibility and the color was merely an extra affordance? Anyway please get this fixed in the next 2/3ish weeks otherwise, release is really a freeze :P

Bojhan - well Dries did say yesterday Oct 15 is likely release date and the wider problem is that we are dealing with regressions and additional bugs in several issues already - its just a general problem we need to be aware of.

I can see the about the border - use a low contrast background color with a higher contrast border. Not really sure its that important - we have to trade off somewhere and I think its better the links are as accessible as can be (color and contrast). In other words, maybe its better to just have a border and no background color?

@Jeff I am aware of it.

We can have only a border, I would prefer to have a background too as it creates an extra level of distinction, is the accesiblity trade off not fixable by changing the shade of the background?

No, because it actually fails on white:

#288CC9 on #ffffff = 3.69:1
#288CC9 on #f9f9f9 = 3.5:1

We need 4.5:1 to pass Level AA for small text, 3:1 for large text. The logical fix is to increase the contrast of the link color, not by much, just enough to creep over 4.5:1, something along the lines of #007BC3, the issue with that is that it changes the feel of the design, would prefer Jen looks at this and has some dialog on the blue.

Ok, well track her down at the sprint then - I think it makes completely sense to fix the real issue, rather than to create a new follow up

On a sidenote: there is another issue (#890362: Links should not be indicated by color only) about the readability of links in Bartik. We decided to not touch the Bartik layout, but only add an underline on :hover and such. Suppose we'd chosen to make links bold (like on drupal.org and BBC.co.uk), this issue would also be solved. I suppose (Jeff?) that bold text validates as large text as well? So that 3:1 would be enough? If so, we could still use the background on sticky nodes.

This is about contrast only, this issue would not be solved bold or not bold it wouldn't have enough contrast. When we fix that we can keep the background.

#13: bartik-sticky-styling.patch queued for re-testing.

OK - looks like we need to review this since the default link color changed in #908966: Bartik's default link color has inadequate contrast to background (WCAG 2.0 violation) and we need to remove styles for the full node view - as yoroy points out sticky is a contextual thing.

and we need to remove styles for the full node view - as yoroy points out sticky is a contextual thing

isnt this needs work?

The contrast ratio passes WCAG AA, so were good for the background color of #f9f9f9.

Problem here is IE6. Heres our options:

1) Chain the selectors such as .node-teaser.node-sticky {}, this will correctly style sticky teasers, but will style the full node view in IE6 as well. Other browsers (IE7+ and everything elese) will correctly only style the teaser view.

2) Add a body class so we know we're either looking at teasers or the full node, so we could use something like .page-teasers .node-sticky {} which will work for all browsers.

3) Screw IE6 and simply override sticky styles in ie6.css and remove the border and background, IE6 gets no sticky styles at all.

I would go for 3, since IE6 is nearly dead and you're having a real crappy experience as it is...

Any time I can vote for "screw IE6" I will ... it's certainly screwed me enough times

Rerolled based on suggestions by bleen and Jeff Burnz. Styles only sticky teasers and overrides IE6 to show no special styling. I haven't had time to test in IE6 but I think it should work. Uploading in case someone else comes through and can test. If not, I will be able to later this evening.

I think we need to remove the padding-top, looks like too much space above the title.

BTW - We need to reset the padding as well for IE6.

Rerolled to remove padding-top and to reset IE6's padding altogether.

The previous patch removes the border-bottom from the teaser as well, so this reroll puts that back in IE6.

@rjgoldsborough - can you take a look at this, I just did a quick re-roll while I was playing around with your patch - the slight difference in alignment between the block and sticky node was annoying me something bad... lol, I just made an adjustment to fix this.

Re #34: The margin between the links and the bottom border is different for sticky and non-sticky nodes in IE6. Screen attached.

This patch is the same as #34 but resolves the wrong padding-bottom for IE6.

Ok, this looks good to me.

Design good, code good: yes, rtbc

#36: bartik-sticky.patch queued for re-testing.

I'm not sure exactly why, but the latest patch no longer applies. Could we get a re-roll?

Glad it didn't apply, it still had #block-system-main code in there that was just removed by #660614: Remove #block-system-main dependency, fix font sizes, remove crufty CSS.
Easy re-roll, please RTBC!

back to RTBC (once its green of course)

Great. Committed to HEAD. Thanks!

There is a bug in this last commit. In IE6 and IE7, we now have an additional 10px margin on top of every div.node-teaser (see screenshot bartik-sticky-before.png). The problem with IE6 and IE7 is that the margin-top of 0.5em on h2 inside the div.node-teaser doesn't collapse as it should because it's nested. This also leads to an excessive distance between subsequent teasers.

I fixed this and also found a solution to display stickies in IE6 which is included in this patch.

I attached screenshot comparing FF/Win with IE6/IE7 when stickies are present (bartik-sticky-after-with-stickies.png) and not (bartik-sticky-after-no-stickies.png).

removing tag.

Wont this result in the the full node getting sticky styles in IE6? We cant have that because the it will give background to the comments, which is not what we really want and the comments triangle thingee has white background (its not transparent, why not I dont actually know).

Discrepancy of margin is not major, we can downgrade this now.

@Jeff: I have no time to further test this today, but adding body.page-node div.node-sticky in ie6.css should do the trick since .page-node is always set on the body tag when viewing a full page node.

I will report back.

Waiting for a report back :)

tagging "Needs IE test"

(tagging for me to look at while working on #827530: Content in Bartik Highlight and Help regions doesn't look to hot and #827528: No styling for PRE, CODE tags in Bartik )

Hate to be the bearer of bad news, but the previous patch does indeed break IE6 and looks much worse then an extra 10px I think.

Here is an updated patch for IE. I found a class that seems to be applied only to full page nodes: 'node-page-' and used that to fix ie6's lack of support for multiple classes.

I also added a small fix to form items in the comment box.

There is a problem with textarea widths across all IE's (even in D6 i think) but i will deal with that in a separate issue.

Looks good to me. Styles the sticky in a node list but not on the full page. And also fixes the IE form bug.

@52 - the problem with text areas is the width=100% on text areas and the use of box-sizing property which IE does not support.

@Jeff, I know :) I will open a new issue and suggest a width=98.5% for all IE versions.

+++ themes/bartik/css/ie6.cssundefined
@@ -7,16 +7,23 @@
+.page-node- .node-sticky {

This should not be seen as a "needs work" type comment ... just a "someone please explain this to me..."

What is the reason for having both the page-node and the page-node- classes?

Other than my lack of familiarity with this class ... the patch in #52 looks good to me, but I'd like to hear from Jen and/or Jeff before marking this one as RTBC

Powered by Dreditor.

.page-node- is a generic .page-node-NID class, so you know you're on a specific /node/NID page.
.page-node is used on any /node page, which includes the default homepage.

The one thing that irks me about the patch in #52 is that is relies on a bug - the "page-node-" class is an artifact of the way body classes are generated from the suggestions, its inclusion is not intentional - so what worries me is that one day it might get "fixed".

I'll throw something out here - we can add a "node-full" class and use it to override .node-sticky styles, I actually think this should be in core, but that's another story....

If we want to go with the "page-node-" class I'd be reasonable happy with that, but, if this were my theme (aka a contrib theme) I would add the new class without hesitation.

See, I thought that .page-node- was probably a bug, but I didn't want to say anything since it was so useful...
I'm okay with Jeff's patch.

I agree that adding a new class for full node display is a lot better, but I didn't know we were still allowed to do that at this point :)

Also, you forgot to include the small fix I threw in for the comment form in IE6... see attached screenshots.

This is by far the more logical way and if we can still get it in this late we should.

This patch is mostly about fixes after the initial commit in #43 so marking RTBC.

Agreed. That .node-full rule is amazingly helpful, and much more clear/precise.

Committed to CVS HEAD. Thanks.

Seems to me that this entire funtion in template.php is superfluous:

<?php
/**
 * Override or insert variables into the node template.
 */
function bartik_preprocess_node(&$variables) {
  if ($variables['view_mode'] == 'full' && node_is_page($variables['node'])) {
    $variables['classes_array'][] = 'node-full';
  }
}
?>

It only conditionally adds the node-full class. This class is used only once in all of Bartik's css files (yes I checked), and IMHO this creates more problems than it solves. It can confuse developers for one. Anyway, here's the piece of CSS where it's used:

.node-sticky {
  background: #f9f9f9;
  background: rgba(0, 0, 0, 0.024);
  border: 1px solid #d3d7d9;
  padding: 0 15px 15px;
}
.node-full {
  background: none;
  border: none;
  padding: 0;
}

See what's happening here? The node-full class is only used to unset background, border and padding which has been set by the node-sticky selector above it and nothing else! It also creates problems, when a contrib module might set a border/background on a node in full view, where Bartik unsets it again! Example: #1194792: Margin below post info and topic has no border. (Blue Lagoon)
Instead, the selector above it should look like this:

.node-teaser.node-sticky {
  background: #f9f9f9;
  background: rgba(0, 0, 0, 0.024);
  border: 1px solid #d3d7d9;
  padding: 0 15px 15px;
}

This is inline with the css above it, which are all teaser styles. And this way the css will never even be applied to the full view in the first place, making the node-full css superfluous and consequently also the bartik_preprocess_node() function in template.php

Even if this is unacceptable for reasons I cannot fathom, then the node-full selector is still unnecessary and should be changed to this:

body.page-node- .node {
  background: none;
  border: none;
  padding: 0;
}

Still enabling the removal of that function in template.php I referred to earlier.

Note that double selectors like .node-teaser.node-sticky won't work in IE6. For what it's worth.

Typically we avoided chained selectors in D7, for the reasons stated in #67.

This can be changed to a chained selector in D8, as in .node-teaser.node-sticky {...}

This is a won't fix for D7, I'm not going to potentially force an upgrade on modules and subthemes for such a minor issue.

Glad we're dumping that anchor (IE6) in Drupal 8 then :)

Seems #66 could use an updated patch?

Updating with a patch using suggestions from #66.

I didn't delete the whole bartik_preprocess_node but did delete the part that set the class. I would think leaving it there for a beginner to see would help. If not, an easy re-roll.

I've also switched to using chained selectors so only the teaser view of the sticky gets styled, and not the full.

Three good novice tasks here:

1. #71 needs to be rerolled on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.
2. Let's get some before and after screesnhots for #71.
3. Let's do cross-browser testing to ensure the style appears correctly, especially in IE 7 and 8.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Rerolled. Images coming.

+++ b/core/themes/bartik/template.php
@@ -104,9 +104,6 @@ function bartik_process_maintenance_page(&$variables) {
 function bartik_preprocess_node(&$variables) {
-  if ($variables['view_mode'] == 'full' && node_is_page($variables['node'])) {
-    $variables['classes_array'][] = 'node-full';
-  }
 }

That whole function should be removed.

28 days to next Drupal core point release.

Updated patch to remove empty function and comment.

The actual patch looks wrong but it appears to apply correctly.

+++ b/core/themes/bartik/css/style.cssundefined
@@ -596,17 +596,12 @@ h1#page-title {
-.node-full {
-  background: none;
-  border: none;
-  padding: 0;

This hunk should probably be backported to D7, as it breaks the background of .node-unpublished.

15 days to next Drupal core point release.

Er, dreditor fail.

See 68 this is not going to be backported...
And the other issues will be fixed in http://drupal.org/node/1217012#comment-5360940 very very soon.

So i'm going to close this as a duplicate.

This hunk should probably be backported to D7, as it breaks the background of .node-unpublished.

This is being discussed at #1854376: Bartik overrides unpublished style from node.css - probably best to use that as a dedicated issue for it. I'm reverting this issue to its original status (since the original patch committed here was not a duplicate of anything else).

We should try to come up with a D7 fix for this if at all possible (the lack of node-unpublished styling in Bartik is a pretty noticeable bug).... But I'm especially looking for reviews from people who participated in this issue, to see if it's possible to do that in a way that doesn't break anything.