Problem/Motivation
the comment template should follow the same markup & css patterns as node.html.twig
Proposed resolution
{%
set classes = [
'comment',
'js-comment',
status != 'published' ? status,
comment.owner.anonymous ? 'by-anonymous',
author_id and author_id == commented_entity.getOwnerId() ? 'by-' ~ commented_entity.getEntityTypeId() ~ '-author',
'clearfix',
]
%}
<article{{ attributes.addClass(classes) }}>
{#
Hide the "new" indicator by default, let a piece of JavaScript ask the
server which comments are new for the user. Rendering the final "new"
indicator here would break the render cache.
#}
<mark class="hidden js-hidden" data-comment-timestamp="{{ new_indicator_timestamp }}"></mark>
<footer class="comment__meta">
{{ user_picture }}
<p class="comment__meta__submitted">{{ submitted }}</p>
{#
Indicate the semantic relationship between parent and child comments for
accessibility. The list is difficult to navigate in a screen reader
without this information.
#}
{% if parent %}
<p class="parent visually-hidden">{{ parent }}</p>
{% endif %}
{{ permalink }}
</footer>
<div{{ content_attributes.addClass('content') }}>
{% if title %}
{{ title_prefix }}
<h3{{ title_attributes }}>{{ title }}</h3>
{{ title_suffix }}
{% endif %}
{{ content }}
</div>
</article>
Remaining tasks
test
screenshots
User interface changes
none
API changes
markup changes but is not closed before RC1
Proposed markup for comment template
As a part of #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme
proposed template thats in sync with markup & classnames as in classys node.html.twig
Please note, we need to keep the variables in sync with the node template:
#2004252: node.html.twig template
Part of:
#2289511: [meta] Results of Drupalcon Austin's Consensus Banana
Before patch
After patch
Comment | File | Size | Author |
---|---|---|---|
#135 | interdiff-115-135.txt | 1.86 KB | mortendk |
#135 | comments-compare-135.jpg | 1.14 MB | mortendk |
#135 | markup_for-comment-2031883-135.patch | 11.48 KB | mortendk |
#134 | interdiff-2.txt | 1.33 KB | mortendk |
#134 | markup_for-comment-2031883-134.patch | 11.39 KB | mortendk |
Comments
Comment #0.0
jenlamptonblocked
Comment #0.1
jenlamptonremove links hiding and comment explaining why
Comment #1
minneapolisdan CreditAttribution: minneapolisdan commentedSince we're being HTML5 aware, we probably should add a
<header
tag as well:Comment #2
mradcliffeThere was some discussion over in #1965650: Comment rendering broken: comment body rendered outside of the bubble to improve Bartik's comment markup to use author and datetime elements. Also note that core comment.html.twig has the mark element and Bartik's does not.
The markup developed in that issue for Bartik was going to be:
Comment #3
jenlamptonwas there a reason this was the markup for Bartik and not for core? What do we NOT want in core that's in here?
Comment #4
andypostNow comment settings is a field, also realted #1962846: Use field instance name for header of comment list, drop comment-wrapper template
Comment #4.0
andypostauthor view mode
Comment #5
star-szrDon't mind me, just getting rid of the hash on the dreammarkup tag to keep things consistent, and D7 d.o might have some issues with tags with '#' in them as well.
Comment #6
pakmanlhI don't understand if this proposed markup has to be implemented just as it is, because the actual markup contains stuff like hiden class on mark new tag that would be lost... And the new view_mode bio doesn't exists yet... Maybe this issue is blocked by other by the moment...? @andypost can you give more feedback about that please? Thank you!
Comment #7
andypost"new mark" now added via JS
view mode seems makes no sense now
Suppose better to start from #1962846: Use field instance name for header of comment list, drop comment-wrapper template
Comment #8
mortendk CreditAttribution: mortendk commentedtook a stap at the comments and cleaned up all the classes thats added in & put them into the template
removed the hardcoded class & added it into the template
status is allready available for the template so its created inside the template
is this ever an issue anybody have used for anything ? removed cause the rule of theming for the 80% usecases
again for the 80% usecases
die
put into the template
Comment #9
mortendk CreditAttribution: mortendk commentedComment #10
andypostStart testing...
Comment #12
MathieuSpil CreditAttribution: MathieuSpil commentedComment #13
MathieuSpil CreditAttribution: MathieuSpil commentedAltered the patch so the CommentCSSTest.php would no longer include this:
Because those 2 classes are no longer used.
Also changed CommentPreviewTest.php from:
to:
Still a todo:
Please note, we need to keep the variables in sync with the node template:
#2004252: node.html.twig template
Comment #14
MathieuSpil CreditAttribution: MathieuSpil commentedComment #15
pwolanin CreditAttribution: pwolanin commentedComment #16
andypostSuppose it's time to think a bit about
comments formatter
too, without a big picture how comments would be rendered this issue will cause another major to properly work with render cache.The intermediate level of render it's views blocks and formatter widget, also comment should be able to be JSON serialization into frontend
themer
(suppose a backbone template).Currently we have a modal piece of render (comment entity list + comment form) so comment wrapper needs some love too.
Also entity link can open modal with comment list...
Comment #17
andypostI suppose wrapper should gone in favor of formatter template or render function with caching
And other links to comments should work through #2002094: Improve performance of comment.html.twig
should gone a whole.
is there equivalent in twig template?
This check needed to ensure that preview have user picture so selector is wrong
This make sense but there's special field instance setting
any actual purpose to change to this kind of translation?
Also now we have entity operations #2165725: Introduce hook_entity_operation()
Comment #18
mortendk CreditAttribution: mortendk commentedthis patch should fix the classnames to follow the code standards
Comment #19
mortendk CreditAttribution: mortendk commentedbot get to work
Comment #20
andypostWe are trying to minimize this usage in #1857946-11: Comment parent template variables are built twice
Comment #22
LewisNymanComment #23
larowlanWe should also be looking at the template suggestions after #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form see #2282205: Revisit comment template suggestions
Comment #24
andypostPatch outdated after #1962846: Use field instance name for header of comment list, drop comment-wrapper template
there could be more then one comment fields on page now
Comment #25
mortendk CreditAttribution: mortendk commentedComment #26
meichr CreditAttribution: meichr commentedI'm going to test this one.
Comment #27
andypostPlease make sure about changes in related #1857946: Comment parent template variables are built twice
Comment #28
meichr CreditAttribution: meichr commentedUnassigning for now.
Comment #29
lauriiiComment #30
euphoric_mv CreditAttribution: euphoric_mv commentedI can see that proposed markup in issue summary is much different then markup in discussion later.
For now, it's not clear what to do. Can you please update issue summary so someone can start/continue/finish this issue as well?
Comment #31
andypostIS should be updated still, but here's a result of discussions within Goa code sprint
Comment #32
andypostin related issue comment links are moved to comment content
Comment #33
Wim LeersLooking forward to @mortendk's review :)
LOL! WTF, D8 HEAD!?
Comment #34
rteijeiro CreditAttribution: rteijeiro commentedFixed some Twig comments. Also pinged @mortendk to have his approval ;)
Comment #35
joelpittetRe #33
My guess is that mark tag was supposed to be after the prefix though shows if the title is missing too. Though not sure if that is needed structure or not, i'd have to guess not if it works without a title too.
Comment #36
andypostOnce links goes to content and #1548204: Remove user signature and move it to contrib then I see no reason in footer, also we need to care about "threading" (div indents) #2318579: Remove comment_prepare_thread()
Comment always have a subject and in case of absense of comment body the subject would be (No subject)
Comment #37
andypostUser signatures gone #1548204: Remove user signature and move it to contrib
Comment #38
siva_epari CreditAttribution: siva_epari commentedPatch rerolled
Comment #39
siva_epari CreditAttribution: siva_epari commentedComment #40
star-szrThanks @epari.siva, the patch applies but the patch needs to be updated:
These bits need to be removed.
Comment #41
siva_epari CreditAttribution: siva_epari commentedSuggested changes included.
Comment #42
siva_epari CreditAttribution: siva_epari commentedComment #43
nguerrier CreditAttribution: nguerrier at Skilld commentedRerolled, as last patch contained references to User signature, but it's gone according to #1548204: Remove user signature and move it to contrib
Comment #45
andypostthis change is wrong, it's footer closing tag
Comment #46
nguerrier CreditAttribution: nguerrier at Skilld commentedPatch updated with @andypost comment.
Comment #47
nguerrier CreditAttribution: nguerrier at Skilld commentedComment #48
andypost@Cottser now it looks ready
Now we need update summary and RTBC
Comment #49
andypostAnd how it looks with patch in standard profile - bartik
Comment #50
andypostNo interdiff because here's a code merged in from #2428509: Comment links weight should be managed by view display
1) updated summary with latest module provided markup
2) added screenshots before-after
3)
.comment-footer
unused selector (it was also) so maybe more clean-up possible4)
.comment__text h3
is needed to remove default H31em
@todo #49 data type still needs debug
Comment #52
andypostAlso I think we need to swap
comment__text
andcomment_content
PS: I mean code merged from #2428509: Comment links weight should be managed by view display
Comment #53
zestagio CreditAttribution: zestagio commentedI removed some no needs markup
Comment #54
zestagio CreditAttribution: zestagio commentedComment #55
zestagio CreditAttribution: zestagio commentedComment #56
zestagio CreditAttribution: zestagio commentedRTL styles broken #2480497: Comment rtl styling broken . Fixed with patch.
Comment #57
Manjit.Singh@zestagio commenting in [rtl] is looks good for me... Also i have one concern, How do we fit it in small screens ?
I had captured a screenshot in small screen,
Input
field is getting disturbed in it. Can you please check the screenshot.Comment #58
joelpittet@Manjit.Singh can you explain "disturbed" in what way. The screenshot looks fine to me for that input you highlighted.
Comment #59
Wim LeersBump. Looks like this is actually ready?
Comment #60
lauriiiWe need issue summary update
Comment #61
mortendk CreditAttribution: mortendk as a volunteer commentedif the new class is getting modifed or used by javascript we should call it "hidden js-new new" according to the js & css issue, afaik the class is expected to be modied by js
Comment #62
mortendk CreditAttribution: mortendk as a volunteer commentedso thers a couple of issues here:
* the use of
header
is not correct that should be a<footer> i
nstead just as we doing innode.html.twig
(if theres a need for bikeshed look at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/aside & https://developer.mozilla.org/en-US/docs/Web/HTML/Element/header + the node template discussion on using
<footer>
: #2004252: node.html.twig template)* the js used classes needs prefixing & the new class should be removed from bartik, to keep it the same.
*
.comment__meta
is added as well to keep it aling with node.html.twigI think i fixed it all but it need screenshots + test & issue summary
Comment #63
mortendk CreditAttribution: mortendk as a volunteer commentedupdated the issue summary
Comment #64
mortendk CreditAttribution: mortendk as a volunteer commentedComment #65
joelpittetLooks like a nice clean-up, here is a quick pass at a review:
Any chance the hidden should stay there and just remove the js-hidden?
With one class it would be nicer to just put it in the addClass method, also more performant as it doesn't have to create a local variable.
Don't you want to remove the 'hidden' class from there?
Oops? Left over comment?
Comment #66
mortendk CreditAttribution: mortendk as a volunteer commented.hidden
we use .hidden as a class in drupals core css, and thats where the magic happens so we should keep that classaddClass('js-comment')
true gonna fix... i dunno what goes on in bartik but yes *burn* ;)
Comment #67
mortendk CreditAttribution: mortendk as a volunteer commentedfixed as comment on #66
Comment #68
mortendk CreditAttribution: mortendk as a volunteer commentedand interdiff *sorry*
Comment #69
rachel_norfolkAll the comments by @joelpettit at #65 appear to have correctly been implemented.
I have added some comments to the article content type and it does appear to be creating good output. See screenshot:
I think it's good to go.
Comment #70
joelpittetSorry I'm still a bit confused what we are doing here?
Here's where it's confusing. The
hidden
class has the functionality of show/hide an element. (presumably)And here we are replacing it with
js-hidden
, which doesn't show/hide the element. Which basically makes this break?Then here we are removing
js-hidden
and thehidden
class. Why not justjs-hidden
in this case?Thanks for the screenshots @rachel_norfolk
Because this is JS functionality it would be nice to show that (not sure what tool people are using for those cool little animated gif screencasts but that would be nice here)
Comment #71
rachel_norfolkI'll see if I can show it in an animation. (I'm a whizz with ffmpeg ;-) )
As I understand it, the initially outputted html does not show the "new" marker on new tweets as that would break the cache for the page. We use js to work it out and then show/hide as necessary per comment. This does make it seem weird on first output, though.
Comment #72
rachel_norfolkAha - it appear you were right to be confused, @joelpettit!
The idea of the comment-new-indicator.js is to search for the appropriate new indicators and switch them on by removing the hidden class (as well as adding the correct "new" text). Trouble is; we were removing the js-hidden class which is there simply as a handle for js to find the location more easily - it shouldn't ever be removed.
So, attached is a tiny, tiny patch to make that correction.
Also, a fancy gif...
Comment #73
rachel_norfolkComment #74
rachel_norfolkComment #75
Wim Leers#72 was exactly my concern, and it fully addressed it :) Thanks, @rachel_norfolk :)
(I brought the JS-based "new" indicator to core — before it was utterly and completely breaking cacheability.)
But, given that, I have my doubts about some of the changes in this area. Apologies if this is documented clearly somewhere — please point me to the docs if I'm mistaken.
Does this really need to remove both of these classes?
It does, but only because both classes are added in the Twig template. But the
hidden
class is there really for the CSS associated with it, not for the JS to discover it. So shouldn't it therefore be justhidden
?Same here.
js-comment
is used as a selector, so therefore it is justified, unlikejs-hidden
.Again.
Comment #76
mortendk CreditAttribution: mortendk as a volunteer commentedok look like i was overthinging it when i dropped in js-hidden ;)
removed
.js-hidden
it againComment #77
Wim LeersPerfect :)
Comment #78
lauriiiComment #79
andypost+1 to RTBC
my only concerns about this part in context of #2455211: Comment field displayed last regardless of assigned weight
but this out of scope because requires new preprocess function in bartik
Comment #80
rachel_norfolk@andypost - yes, that will need to be addressed in #2455211: Comment field displayed last regardless of assigned weight (which will probably need a re-roll once this is committed)
RTBC+1
Comment #83
rachel_norfolkI *think* it was just a testbot having a moment. Will keep an eye for the result of a re-submit...
Comment #84
mortendk CreditAttribution: mortendk as a volunteer commentedshakes fist at testbot
Comment #87
mortendk CreditAttribution: mortendk as a volunteer commentedafter testbot got sober im gonna set it back to RTBC as it was in #77
Comment #88
alexpottThere are some pretty big visual changes with this patch...
Before
After
Comment #89
Manjit.SinghCSS class was not updated in
comments.css
, so there was some regression issues.Updating classes and adding css. Please verify the screenshots as well.
Comment #90
rachel_norfolkThere appears to be a few differences between the screenshots, both those applied and ones I've recreated. Taking a look...
Comment #91
rachel_norfolkI've adjusted the font-size, line-heights and margins to now visually match current HEAD really quite closely.
Comment #92
rachel_norfolkComment #93
LewisNymanI think I prefer the simpler classes like .comment__author in stead of .comment__meta_author
We should avoid changing the heading styling in different locations and instead use the heading classes introduced in #2400233: Add reusable heading classes to Bartik
Do we definitely need to include the rtl selector here as well? Also can we remove the ul from the selector?
Can we also use the uls from these selectors?
Comment #94
mortendk CreditAttribution: mortendk as a volunteer commented@lewis so if were keeping to the bem naming its correct to use
comment__meta__author
, even that the name gets a bit longer (one of the pitas on bem)If we choose to ignore bem (which i think would be a bad idea) then we should discuss dropping
comment__meta
as a wrapper - Its there to keep us aligned with the node.html.twignode__meta
.I think its important that we look at the overall classes & structure for the whole node (as were expecting comments to be there) - how they are aligning up towards eachother with classnames, so we dont end up having different concepts.
... edit:
looking at it, its what bartik does so it kinda can do whatever right ?
Comment #95
rachel_norfolkNow then. With reference to #93
1. Implemented - it seems sensible.
2. Implemented. This will mean there is a tiny visual difference in screenshots from 8.0.x but anything that encourages a move to rem isn't a bad thing ;-)
3. Implemented
4. Implemented.
Base 8.0.x:
After patch:
Comment #96
rachel_norfolkComment #97
Manjit.Singh@rachel_norfolk thanks for working on this.. There are some minor differences in titles's font size as per your screenshots.
Comment #98
rachel_norfolkSorry Manjit, I should have explained our motivation around the agreed visual difference further in my comment...
You are correct that the h3 title in the comment is not exactly identical to that in the current 8.0.x HEAD. I did adjust it at #91 but, whilst sat with the Bartik maintainer at Frontend United this weekend, we agreed with #93 that it was more important NOT to redefine h3 within the comment css.
Once this patch is complete, we are planning to introduce another patch to redefine how the font-size is declared in Bartik. That will make the difference MUCH easier to cancel out.
I'm setting this back to In Review, if that's okay?
I'm hanging around in IRC #drupal-twig if you want to chat about it?
Comment #99
mortendk CreditAttribution: mortendk as a volunteer commented+1 on RTBC the maintainer of bartik do have knowledge about whats happening & theres a plan then lets move forward (else create a followup issue :)
Comment #100
lauriiiThere is some visual changes on Bartik because of the patch makes comments use the semanticly right html elements but it doesn't look very major or something we should be worried about. The maintainer of Bartik is also fine with this, so am I. Patch generally seems to be working, there is some screenshots showing the visual regression and patch looks clean so I think this is RTBC!
Comment #101
alexpottIn #94 @mortendk pointed out that this was breaking BEM standards - it should be
comment__meta__author
etc. And in IRC @LewisNyman said that "Bartik should also follow BEM standards... eventually.".So I think that when introducing new classes or changing things we should try to follow BEM. Setting to needs review and asking for @emma.maria's opinion.
Comment #102
LewisNymanActually the current patch is correct. It shouldn't be
comment__meta__author
Comment #103
LewisNymanI'm setting this back to needs work because this markup is incorrect, this should be comment__submitted and not comment__meta__submitted
Comment #104
emma.mariaI am aware that this issue is assigned to me. I will take a closer look at this, this evening.
Comment #105
mortendk CreditAttribution: mortendk as a volunteer commented@lewis i have to disagree with you on this ;)
As i read the class organization
.comment
as the main element, so.comment__meta
should have.comment__
to show its inheritence and keep same naming convention as.node__meta
.The sub element of
.comment__meta
cant just be named should then be named.comment__meta__author
, they shuld have the comment__meta__[foo] else the naming & predictability have gone lost.ex: .comment__meta__time, .comment__meta__permalink
If we dont want to have multiple groupings ex:
.foo__bar__baz
as classnames, we should to add that to https://www.drupal.org/coding-standards/css/architecture#formatting and we should then instead name the basecalss.comment-meta
(.comment-meta__author
,.comment-meta__time,
,.comment-meta__permalink
Comment #106
LewisNyman@mortendk An important part of OOCSS is that the class names should not follow the HTML structure. It should matter that
.comment__author
is inside of.comment__meta
. It makes no difference, because it should look the same regardless of where it is placed in the HTML structure.This isn't really a strict rule, that's why it is not forbidden in our standards, there is also no suggestion of one. We should avoid it unless there is a reason why the styling of
.comment__author
relies on the styling of.comment__meta
, like padding or position: relative or something? I don't see that here though.Comment #107
mortendk CreditAttribution: mortendk as a volunteer commented@lewis yes its an important part of oocss to split elements up, its also an important way of documenting where an element lives.
When theres a naturel connection between elements the classname should reflect that else theres basically no need at all for
.parent__kiddo
naming.Were just using the modifers
.foo
&.foo--modifier
instead.My reason for this on the meta is that i compare comments meta the same way as the "media object"
Thats in principal the same we do with the comments meta, why im suggesting
.comment-meta__authorname
, as its part of for a comment, and we should update the documentation for this, so we dont end up with.comment__meta__autorname__lastname--something
classesComment #108
LewisNyman@mortendk Yeah we can update the coding standards. I just want to understand the reasoning behind
.comment-meta__author
, because I can't see why it can't just be called.comment__author
, you should be allowed to move it out of.comment__meta
and you should be able to move any other sub-element inside of.comment__meta
without having to change the class.Comment #109
mortendk CreditAttribution: mortendk as a volunteer commentedcause we define all the author, time, picture etc as "comment meta" just as in the node - Why I se a logical connection between node & comment.
Im not gonna all-in on this, and we could totally do it as a followup, where we align node & comment classes & QA em, cause we are now down in very small details, that really is just stopping progress.
+1 on updating the documentation, else we can end up with
.foo__bar__baz__kazoom__wtf__argh
classes in contrib & we really dont want that.Comment #110
RainbowArray+ 1 to comment__author. Author is an element in a comment, whether or not it is also under __meta. Chaining multiple __element things together gets overwhelming.
I'd also suggest going with comment__submitted rather than comment__meta__submitted for the same reasons.
Comment #111
emma.maria+ 1 to comment__author and not chaining things. The author is an element within comment it can be moved around in any part of a comment and can keep the same class name and styles. We add variants to make things more specific not stack components in class names :)
Also now we have more consensus on markup, I will take a closer look at what has happened to Bartik.
Comment #113
mortendk CreditAttribution: mortendk as a volunteer commentedupdated the patch changed classy from
.comment__meta__submitted
to.comment__submitted
As well i spotted a
.clearfix
in classy that was floating around - it dont have anything to do there, so a little house cleaning as well.Comment #114
andypostExcept few nits good to go, who should fire RTBC?
is this new line needed?
strange indent change
We manage links in UI so this would be fixed in #2428509: Comment links weight should be managed by view display anyway
Comment #115
mortendk CreditAttribution: mortendk as a volunteer commentednid spacing fixed, the indentation is correct & yes lets fix the links in the follow up & get this one out the door
do we need screenshots ?
Comment #116
HOG CreditAttribution: HOG at Skilld commentedTested, all ok. Like in #95
Without patch: http://storage3.static.itmages.com/i/15/0708/h_1436369906_1757861_2a4f66...
With patch: http://storage1.static.itmages.com/i/15/0708/h_1436369580_5137667_b292d9...
Comment #117
HOG CreditAttribution: HOG at Skilld commentedAdd images without link to screenshots.
With patch:
Without patch:
Comment #118
andypost@HOG thanx for screens, fixed IS
Looks we need RTBC from @emma.maria
Comment #119
mortendk CreditAttribution: mortendk as a volunteer commentedawesome then this should finally be ready to roll :)
Comment #120
emma.mariaI'm going to do some visual regression testing and check the Bartik templates right now.
Comment #121
pablo.guerino CreditAttribution: pablo.guerino at Skilld commentedTested and found some minor visual changes, adding screenshots.
Will edit this soon and provide more information about the error
Comment #122
pablo.guerino CreditAttribution: pablo.guerino at Skilld commentedMore information about last comment,
screenshot 1'bug nopatch': before using the patch, the h3 is outside the 'comment__content' class
screenshot 2 'bug patch': after patching, the h3 is inside 'comment__content' and has the font-size applied too
Comment #123
rachel_norfolkHi and welcome Pablo! The visual regression you mentioned above is, as you say, due to the new structure of the html within the comment and (currently) the use of em font sizes. As mentioned in #98, we want to accept this regression for the time being and then have a follow up issue to change the em font size declarations to rems, which will remove the regression.
In the meantime, we might have to live with the tiny font size difference.
Comment #124
mortendk CreditAttribution: mortendk as a volunteer commentedok let's get this to rtbc so we can move further with other issues on comments.
maintainer of bartik is aware 2 times screenshots. we have a rtbc :)
pulling the tricker
Comment #125
emma.mariaI am aware that there are visual regressions and it looks like they can be fixed in this issue.
A good few people reviewing this have pointed out that there are obvious visual regressions so I think we can't just leave it like this. No one wants to raise a follow up and I am pretty sure Core committers will point out the regressions also. I am happy to take a stab at fixing this seeing as no one else wants to fix it at this point.
Comment #126
mortendk CreditAttribution: mortendk as a volunteer commentedI took the last screenshots and compared the two, the visual progression for Bartik is 2px to the left and 3px down.
What I would like is to raise is the awareness of us becoming "our own worst client".
We stop major markup/theme progressions because of 2-3 pixels, that have no effect on the final product (Drupal8)
If Bartik was built with a major design & styleguide then yes it would make sense to build on that for everystep of the way, but its unfortunately not. Does it make sense to be "pixel perfect", when we all now that bartik was never build that way ?
Comment #127
LewisNymanWe shouldn't be introducing regressions now. Drupal is a framework and a product so we shouldn't be lazy about this.
Don't we just need to change the em font size to rems to fix this problem?
Comment #128
rachel_norfolkThat's exactly what we agreed to do at FEU - get this to RTBC and then introduce a follow up that converted to rems.
I'm very happy to help with that follow up, once we are ready to make it.
Comment #129
lauriiiI agree with lewisnyman in some sense; if we change 1-3pxs regression in every issue, after all the issues things has visually changed quite a lot. Problem that I seen in this is that markup cannot be changed after RC is out but CSS can be still changed. Because of that fact I might still suggest to get this in even though there would be a very minor visual regression.
Comment #130
LewisNymanIt seems like a really simple fix. We should do it in this issue.
Comment #131
lauriiiI think so too. I thought CSS could be changed after RC but that wasn't true. So we should fix it here because it is not a good time to cause regressions.
Comment #132
mortendk CreditAttribution: mortendk as a volunteer commented@lewis & @laurii This is not about being lazy - this is about getting progress. We have way of stopping our own progress of getting basic elements in (like cleaning up a template from html4 to html5, so other issues can be fixed) We have turned into the worst possible client for ourself - And thats about until 5 minuts before RC1 and everything will be thrown in with a shovel (as it got done with ex bartik 5-6 years ago)
This is not about everything can be pushed 3pixels (we are not talking about were changing everything from blue to hotpink) its about us creating an enviroment, where we strive for a "perfection" thats not even defined. If bartik was a design that was set in stone, then it would make sense to follow those guidelines to the point - Its not and we all know that.
This rigid "Visual regression" thinking sets us in a pixel perfect state for a design thats old and worn out - If it was & we had a design manual i would not raise this issue, but it seems very silly in the comment template.
Its a huge issue if we cant fix core templates later on... If we turned RC1 in 10 minuts, then we can be sitting here talking about 3pixels, but our template in drupalcore is still html4, and we have to live with that for another X years - In that case im a pragmatic, lets fix the real stuff now and have followup's for stuff like 3px, cause the css margin-left:2px is not a part of the api but
<article class="comment foo bar x">
isComment #133
mortendk CreditAttribution: mortendk as a volunteer commentedSo The font's have been changed overall in bartik, which is afaik not an issue that this patch should take care in this patch ??
the showstopping 2pixels of the comments boxes & the nav tag have been corrected as the screenshot should show (or compare the 2 original screenshots in your image editor or choice)
Comment #134
mortendk CreditAttribution: mortendk as a volunteer commentedFixed an upcoming issue with bartiks css that is not following the css documentation standards in ordering the css.
Comment #135
mortendk CreditAttribution: mortendk as a volunteer commentedDid a new run on the fontsizes that changes cause were using ems in bartik.
Calculated values in this patch for the em values, that imho should be changed to rem, that calculating them is a pita - but would suggest this for a general followup patch in bartik.
comment__content h3
comment__content
These values are barely visible for the human eye, but for those that see this is an issue heres a sceenshot, as its visible even with the same line-height its not the exactly same, but i would be so bold to suggest that were on the way out on the biketrack ..
using screenshots in crome this is the comparison between the 2 files:
Comment #136
lauriiiTested this manually and it looks good to me. Also the comparison looks quite good; not exactly the same still but its _very_ close. Thanks morten for fixing this!
Comment #137
joelpittetI'd be quite happy to see this one land:) Thanks for your efforts @all!
RTBC++
Any improvements could very well go into a follow-up. This gets us improvements for the markup the CSS and js class names.
Comment #138
davidhernandezJust my two cents while casually following this issue. On the subject of follow ups, if there is something that can be done in this patch you should do it here. It does not matter if there was an agreement two months ago to create a follow up. We are at the point where it is getting harder to get issues committed. Avoid any regressions, if possible, because we cannot assume a follow up will get committed.
Comment #139
lauriiiThere is no regression. Its not follow-up that we need to create but instead completely new issue which actually is opened already #2399247: Reduce number of different font-sizes in Bartik for better consistency.
Comment #140
mortendk CreditAttribution: mortendk as a volunteer commentedyup and as long as bartik have these font issues we "maybe" should not change markup for bartik when we fix classy & core simply cause it keeps issues stalling as happend with this cause of its use of em's - Anyways lets get this in so the work on sorting comments can get done
Comment #141
lauriii@mortendk: We should still fight for the same goal (making Drupal as awesome as possible) even though everyone has their own interests. Changing core/Classy markup simply cannot break Bartik or Seven. I would be more than happy to have automated tests for this so it wouldn't be even possible! In the Bartik queues the focus has been heavily focused on fixing these problems so that in future it could be a lot more stable and changes in the core/Classy could be applied easier and it would be more predictable what kind of issues changes are causing. Anyway I'd like to encourage everyone to help on those issues since they would make it easier to fix things in core/Classy.
Comment #142
mortendk CreditAttribution: mortendk as a volunteer commented@laurii were not making a better product, we are wasting valuable time on details that really matter, yes a huge "design" change in bartik of 3 pixels, makes no sense to stop a patch for coming in - maybe its a policy change that we need then, and thats why im raising this issue.
Blindly following pixel perfection on a theme that was never pixel perfect to begin with is stupid. If this was a client project, it would have been pushed to production 3 months ago and a followup later on would then have fixed any "scandalous" 0.01px issues.
We could be way more productive and fast instead we choose to constantly stop at every chance (spelling error in documentation a missing dot whatever else we can find, that could be simple followups) This is frustrating for everybody and in the end makes any kinda of change taking months instead of hours, thats not good for anything.
Comment #143
LewisNyman@mortendk You're not in a position to decide which details matter or not. Can you imagine all the people who have rolled their eyes when you've been making demands over an extra div or whitespace? They are not in a position to tell you that your concerns don't matter and neither are you.
You've spent more than double the time moaning and arguing then it took to actually fix the issues, and you're complaining about productivity? This does not include the time spent by others who have to deal with your outbursts.
Comment #144
mortendk CreditAttribution: mortendk as a volunteer commented@lewis im totally aware im not in that position ;)
That won't stop me in raising the problems for the way we are working. We are developing in a way that isnt as productive as we could be - we have turned out to be our own biggest enemy without even realising it (when anything takes 4 months for changing anything we have an issue)
My "moaning" as you flattering are calling it (...) is on the general level of how we are not moving forward in the speed we could, were hella unproductive. Yes it should be elevated to a meta instead, (cause its policy & not just code ) as this issue with 400 comments in makes alex pots head hurt when this one day hits the inbox of the committers :P
cheers
Comment #145
alexpottThis patch only changes markup and therefore is permitted in beta. Committed d4011a5 and pushed to 8.0.x. Thanks!
Comment #147
nod_