blog comment spacing does not match activity stream spacing

[DJPaul]

There is a difference in spacing/margins around the activity stream item lists, and the blog comments list, the latter of which has been restyled in BP 1.3 to match the activity stream.

These should be as identical as possible. Compare against a BP 1.2.x site to see whether the new blog comments styling needs updating or if this is a regression in the activity stream styles.

UPDATE: Things left to do in this ticket:

• Make reply buttons have the same colours as matching buttons on the activity stream.
• RTL styles for comments. Very important, it's extremely poor at the moment.

[modemlooper]

I think this has to do with the markup and not CSS. The HTML is different for comments and activity replies.

[hnla]

Hmm tried to look at this but can't get user blogs posts to show up in activity streams

[DJPaul]

@hnla -- it's not to do with blog posts in the activity stream. Compare the activity stream list against a blog post's comments.

[hnla]

Oh ok, comparing the comments on a blog post and comments made to activity stream are hugely different though?

[hnla]

matches post comments & activity comments spacing

[hnla]

Issue was with the div enclosing a img.avatar. On activity stream .activity-avatar was not styled, on post comments it had bottom margin and enclosed it's floated children, img.avatar also had a greater margin-bottom.

Patch attached.

Added a ruleset for .activity-avatar and have had to split styles between it and img.avatar to make a match with the post comment styling, and reduce the margin top of the activity-header.

One issue surfaced when a long group title was added for a new test group as it pushed the view.delete links behind the fav button so have added a margin-right to clear it.

This isn't ideal, a margin of 130px was required to clear the possibility of both 'reply' & 'fav' buttons displayed, better solution is to re factor the flow of the activity-meta element so it can be floated on the same line or move the element to the bottom using pos:absolute where it won't interfere with anything.

[DJPaul]

The comments should match the activity stream styles as close as possible and where relevant, not the other way around. If there's a display issue with long group names, that needs to be another ticket.

[hnla]

Reversed out the change and started over. this way round is a little harder than matching activity to post comments in this sort of detail as the two aspects start to differ quite a bit in how they are marked up and styled, but this version gets close to a match but you might want to think about re-positioning the post comment reply button.

Removed the correction for activity-meta hiding behind fav button.

[hnla]

version two matches post comments to activity commnets

[DJPaul]

Thanks hnla. I had a quick look this morning and it makes things look much better. While looking, I noticed some of the margins on the activity stream looked unbalanced. I'm going to tweak those on top of your patch, and commit in one go.

[hnla]

Well there are a fair few minor anomalies that I noticed but to start adjusting those might have opened a can of cascading worms and started to become a little involved. I spotted a few contradictory rulest properties, they don't exactly manifest as issues but do highlight the fact that the sheet is starting to become a little bloated and conflicting.

If you are adding in tweaks on top of this patch you might as well add back in the activity-meta issue I mentioned and corrected?

[djpaul]

(In [4424]) Normalise activity stream and blog comment styles. Props hnla for initial patch, see #3242

[DJPaul]

There's more to do, but the first pass is in. Pictures or patches appreciated. Two items I am aware of that need to be addressed are:

• activity stream, replying to a nested item -- should the avatar in the reply area be large or small? (compare to how a nested blog comment reply works)
• activity stream, nested items -- afaik these are only comments in core. Therefore, should they say "[admin] replied: [May 24, 2011] [Reply]", like the nested blog comments?

[hnla]

Small & yes

[Dennissmolek]

Moves blog reply buttons to be like activitys..

[Dennissmolek]

My patch moves the reply button from under the comment to be inline with the meta info. I think it was underneath as a blog function expecting longer comments on blog articles.
There is no class tag on the P element created by comment_text() so I had to target the first P tag, There may be a better solution such as :firstchild but selecting all other P's under the children list worked well.

Second thing I did was change the order of the edit/reply in nested comments. This is to keep up with how the first comment is displayed.

[Dennissmolek]

View of how it looks after the patch

[Dennissmolek]

Revision that creates Reply buttons instead of simple link

[Dennissmolek]

preview of patch

[hnla]

Dennis looking at the functions file patch it looks as though you have moved the edit link into the elseif comments_open() block. The edit link should appear regardless or at least should appear regardless of whether comments are open or closed as it edits the existing comment.

I may be reading things wrong though as I'm viewing three different versions at the moment.

[hnla]

My bad sorry Dennis I see why you have moved it now.

Paul did that block really need the rewrite? I simply added a test around the actual stray middot otherwise the block functioned ok didn't it? doing the elseif means edit link needs duplicating, not that it's really an issue, guess it reads better with your reworking of the original block.

[DJPaul]

(Hnla, I assume we're discussing r4419) Sure, the original patch worked fine. It's just a code style issue; comments_open() bails out if comments are closed. As we only needed the middot if comments are open, and because there's nothing else inside that IF, it made things a little neater.

[hnla]

Yes r4419 I't does make more sense and I been slightly conscious recently that some conditional blocks here & there are quite difficult to follow the logic and flow of. I personally have mastered the art of writing terrifying convoluted conditionals that I can never read my way out of :)

[hnla]

This is just a aide memoir for issue remaining from #3249 - comment replies removal of div.comment-options if comments off.

div spuriously displayed even though comments off and thus comment_reply_link() returning false resulting in an empty element.

removal results in the margins that it carried being lost and comment spacing affected.

Basic solution would be to take the bottom margin that exists on div.comment-options and transfer it to the ul.children directly below as a top margin(currently no top margin) this accounts for the margin between these two elements always present. .comment-options top margin would need to transfer to the bottom of the last-child p of the comment_text()

However looking at this I don't get the markup, it flows comment-meta and comment-options in div.comment-content, why!! why are those two items not outside of content or why isn't there a div.comment-entry this would make life simpler, does it have backwards compat issues? if not I suggest a re-formulation of the markup to include div.comment-entry

<div class="comment-entry">

<?php comment_text() ?>

</div>

div.comment-content is actually a redundant element it aggregates it's children for no good reason the li parent already acts as the aggregator.

[Dennissmolek]

@hnla

if you apply the patch I dont think the margins would still be an issue.

Second, the <LI> works as a separate container so I think its ok.
I think comment-content is needed, when you introduce the children div as well as the avatar div, splitting out the actual content is useful.

I agree about putting the content itself within another div. The activity stream is almost exactly the same and does this..

I think another issue is the semantics we are using. For some reason the activity stream uses activity-header instead of activity-meta, then it uses activity-meta the same way blogs use comment-options.

Wouldn't be an issue, but the threaded comments go back to the blog style format(assuming they used them as a template)

I suggest we change the name of activity-header to activity meta, activity-meta to activity-options...

[hnla]

Think you're probably right, margins might not be an issue, before presentation can be addressed the final markup views need agreeing on then it's safe to look at any margin/padding issues.

I don't think for one moment it would be safe removing elements for fear of breaking things, yet there is good case for that div vanishing, the li element is the parent container and comment-content is a somewhat unnecessary bit of semantics, yes we know it's the content what else could it be there's nothing else at a level above in that parent! This for me raises the issue of bp-default falling foul of that disease divitis, of over markingup layouts.

I would agree with your name changes but also that what we appear to be highlighting is possibly a need for these various areas to be looked at and brought into line with each other also I would like the .comment-entry added even though it does counter my desire for leaner markup.

[DJPaul]

Changing the names of the classes in the activity stream is likely to break any child theme which has customised it. Adding a new DIV or class or whatever should be okay as most of the activity stream markup is hardcoded in BuddyPress.

I'm loosing track of what all these patches are for!

1) Changing the reply link to look like a button is an enhancement, and probably should go in a separate ticket so it can be discussed.
2) Adding a div.comment-entry around comment_text() is okay. Is everyone happy with that class name?

[Dennissmolek]

@DJPaul
My first patch(move reply buttons) moves the reply button to the same row as the meta information for blogs. It's the same way the activity does it. It also moves the reply button to the far right which is in correlation with the "parent" comment. So that would be in line with this ticket.

The buttons IMO look better, but you're right it should be discussed. I'll open another ticket later.

The class name isnt a big issue, but if bp-default were ever to be re-done, uniformity would help.

The class name sounds proper, but if we are trying to keep it closer to activities it would be comment-inner.

[hnla]

In terms of posts the post is div.entry so it just seemed logical that with comments it was .comment-entry.

Should it in fact be brought more into line with activity stream? I find activity stream markup to be convoluted and difficult to read through. .activity-inner is not really that great a bit of naming as it says nothing of what the data is, it describes a vague possible positioning context.

I think that this particular addition could be sidelined if no ones sure whether it is worthwhile, from a theming point of view I can hack that block and add the comment-entry and remove comment-content leaving comment-meta at a level I find more appropriate.

[djpaul]

(In [4429]) Second pass at normalising activity stream and comment styles.
Don't generate an empty div; correct usage of comment_reply_link().
See #3242

[DJPaul]

My last commit stopped the comments outputting an empty 'comment-options' div in some situations (found another case that r4419 didn't fix).

[DJPaul]

for Dennis

[hnla]

My last commit stopped the comments outputting an empty 'comment-options' div in some situations (found another case that r4419 didn't fix).

What were the situations where it didn't stop it? (I've updated my checkout but haven't gone over it detail, but will look in the morning.)

[djpaul]

(In [4430]) Tidy up RTL CSS file. See #3242

[DJPaul]

Replying to hnla:

What were the situations where it didn't stop it? (I've updated my checkout but haven't gone over it detail, but will look in the morning.)

When nested comments were disabled.

[hnla]

Ah yes, this is the problem there's always a condition overlooked, well as I often tend to find; you think you've covered all bases but...

[DJPaul]

3242-01.patch is my latest attempt. There's some strangeness: div.comment-entry is not floating correctly (vertical); it's something to do with the image in div.comment-avatar-box. div.comment-entry then needs margin-bottom. And the now right-aligned reply button needs the orange styling rather than the blue.

[Dennissmolek]

Removes blue button selector and makes all replies as buttons

[Dennissmolek]

Should be the last one..

[Dennissmolek]

With Final Patch applied

[hnla]

That seems good to me

[DJPaul]

problem with 100% widths and margins

[hnla]

Not! the right place for this comment but it will have to do as it doesn't feel as though it merits a ticket?:

Noticed that a few instances in default styles where the border-radius property is used it has only been set using vendor specific prefixes, it must be described with 'border-radius' completing the group following after the vendor prefixed property decs.

[DJPaul]

Create another ticket and patch it :)

[hnla]

patching it at the moment - difficult to do though as it's a very mixed bag at the moment last few rulesets look as though they have decided to add border-radius in at the top and separate the vendor specific at the end of the ruleset , tre confusing and not how I would o it but going to have to read the preferred wp ordering and probably start all over - that will merit a few curses!

[Dennissmolek]

What is needed in this? 3232-done I thought was it. Did I forget something? Just trying to clear up the bugs im following..

[DJPaul]

3242-02.patch has problems with 100% widths, margins & overflows. Overflows are easy to see (content cut off), widths show up when using a DOM inspector (i.e. firebug).

[karmatosed]

I have attached 3 styling colour suggestions for the post author.

My personal choice would be either the grey or if want a stronger one the light yellow hint version.

Here are the hex values:

Grey: #F9F9F9
Yellow: #fffbef
Blue: #f7fcff

[karmatosed]

This does not solve the problem but is a sticky plaster hacky code....

ol.commentlist div.comment-avatar-box {
    float: left;
    margin: 15px 10px 15px 15px;
}

And for the post author background to comment

.commentlist .bypostauthor {
    background: none repeat scroll 0 0 #F9F9F9;
    margin-bottom: 0;
    padding: 0 0 10px;
}

[DJPaul]

width problem with 3242-03.patch

[hnla]

@paul Checking through the patch I notice that empty clearing divs have been added to the bp_dtheme_blog_comments() functions markup these need to be removed, if 'overflow: hidden' causes issues - as it can in certain circumstances - then the clearfix technique ought to be used and rulesets added to style file.

[hnla]

The 'Box Model' describes the box padding , margin and borders as being separate elements to the box 'content' ('width', 'height') thus the dimensions of these elements are *added* to the box content width or height thus you may never describe a block element as width 100% + border width or + margin/padding.

The 100% width should be removed (it's unecessary or should be ) padding removed and overflow used to create a new context on the comment-content element.

​http://www.w3.org/TR/CSS2/box.html#box-dimensions

[hnla]

try this small set of adjustments (I might have missed the reason one of those empty clearing divs was used but dealt with the 'respond' element)

[hnla]

addresses missing reply button

[hnla]

why is

<ul class="children">

Floated? as this is causing the element and it's children to shrinkwrap to content width, is this desired behaviour? removing the float lets the ul element take all the width available i.e span the full width.

[hnla]

If were moving the comment reply link to the top ( a good thing ) I think we need to think about it's class we have removed div class="comment-options" and replaced with p class="comment-reply" this will cause issues with layouts won't it? We might have to keep the same class name 'comment-options' or even, and sadly, the 'div' instead of the 'p'

[DJPaul]

At the moment, this patch and ticket's scope is to get a version of the 3243-0x.patch into trunk. Will review other changes and scope for 1.3 after.

[hnla]

All my comments and revision apply to this ticket and the last patch #3242-03 - I am suggesting no other changes.

Can you test my revision please and my last comment about the element and class name change are to highlight a possible issue that we need to consider before allowing through.

As for the class .children that ul element is floated, this is new and causes the element to shrinkwrap if no width specified, the float ought to be removed.

[djpaul]

(In [4577]) First pass at tidying up comment styling. See #3242. Big props Dennissmolek & hnla

[DJPaul]

I wrote up a huge comment, and I lost it. Sigh. In a nutshell:

• Good work, everyone!
• hnla was right about .children, that float's been removed.
• I've made the border line above/below the comment area to run full width, rather than just above the comment text (to match activity stream).
• Some extremely rudimentary attempts at RTL.

[DJPaul]

karmatosed - re: your screenshots. I can barely see the background colour in images 1 + 3. 2 is good, but insufficient contrast with the yellow of that action button in the screenshot.

[DJPaul]

Updated with remaining tasks

[DJPaul]

Dennissmolek: have I missed anything? Did you suggest moving the reply button somewhere a long time, can you remind us? Thanks

[hnla]

Answering on behalf of Dennis yes this was done and is in the patch and is what I was referring to where we need to look at element changes and classes, the change is a good one but might break themes ? ...or am I getting totally confused, I have been pouring over patched and non to see where the changes were and remember discussing this in detail along with my wish for a new element wrapping comment 'comment-entry' which is also in the #3242-04 patch returned.

Was that patch used or have you made changes manually and committed?

[DJPaul]

Comment styles are all new for 1.3, so there's less backpat issues to worry about.

[hnla]

Attached patch corrects reply button colours

Patch contains just style file, hence named as seen. I dithered over correct approach to further patches i.e whether I should have created a patch following on from last e.g # 3242-05

This ticket is getting confusing :)

[hnla]

As for rtl I started to look but can't see how it's being handled and changes weren't taking effect.

[hnla]

overwrote last patch with revision as noticed #respond avatar box had no margins

[hnla]

match comment reply buttons to activity stream reply buttons

[DJPaul]

3242-correct-reply-button-colors.patch doesn't apply to trunk; I'll pick out the colour change manually. I'll then look at your comments about #respond

[hnla]

Wonder why it doesn't apply? Did a revision go through that I didn't pick up? been wondering about certain things looking wrong when I patch and get the automatic file diff view which shows oddities, darn SVN it can get labored trying to work with it sometimes.

[hnla]

If it helps

line 706 - added selector group '#respond .comment-avatar-box'
ol.commentlist div.comment-avatar-box,
#respond .comment-avatar-box {
	float: left;
	margin: 15px 15px 0 0;
}

line 742 
.commentlist .comment-content div.comment-meta 
p.comment-reply  a.comment-reply-link {
	background: #fff9db;
	border-bottom: 1px solid #ffe8c4;
	border-right: 1px solid #ffe8c4;
	color: #ffa200;
}
.commentlist .comment-content div.comment-meta 
p.comment-reply a.comment-reply-link:hover {
	background: #f7740a;
	border-color: #f7740a;
	color: #fff;
}

funny tabs!??

[djpaul]

(In [4579]) Make comment reply buttons orange. See #3242, props hnla for original patches

[djpaul]

(In [4580]) Correct i18n and RTL for comment styles. Finally fixes #3242