Problem/Motivation
Bartik's code needs to meet current Drupal coding standards.
Proposed resolution
This issue takes a specific section of Bartik's code, it acts as a "component" issue and improves that section of code with minimal impact on the rest of Bartik's codebase.
This issue aims to clean up and properly format the CSS and templates files without breaking Bartik visually.
This issue primarily looks at the css/components/content.css
file.
Work that needs to be included in patches for this issue are fully outlined in the META issue #1342054: [META] Clean up templates and CSS.
The work needs to be crossed off the list below as completed or stated why they were not applicable to this issue in the comments below, to make sure we cover everything.
Also very helpful! Noting the list items in your comment with the patch to show what parts you added to the patch.
Create a patch containing the potential following work:
Code cleanup work
- Check each selector in the CSS file (associated with the particular issue) is in use within core right now.
If not...
a) Check to see if the classes in core have been changed and correct them (for e.g. I found this in this issue ).
or
b) Remove that CSS completely from the CSS file. -
a) Check the CSS selectors are not being replicated in other stylesheets in Bartik.
b) Check the CSS properties are not being overridden by other stylesheets in Bartik.
If a) move all of the properties to the selector in the stylesheet that you think most appropriate for the component you are dealing with.
If a) and b) also remove the CSS properties and values being overridden within that ruleset. - If you find CSS for a component which seems out of place in the file it is currently in move it to the one you think is the correct one.
-
If a selector appears to be too long and/or too specific, check if the selector can be simplified. for eg.
.something .something .something { }
being modified to.something .something { }
. - Check that RTL styles exist when needed and are formatted as per the guidelines. (for e.g. we found that RTL styles are broken on certain pages in this issue, so fix anything you see missing/incorrect in the CSS file.
- If you think the contents of the CSS file could be further broken down into more components CSS files, or grouped together with other existing CSS files to form one component do it. The initial SMACSS issue may not of been perfect, guidelines on CSS file organisation for Drupal 8 can be found here.
- Check the markup from the templates that all of the classes are used as selectors in the CSS files. If not remove them. See an example issue here for this.
Code formatting work
- Add a File comment to the top of the stylesheet - see here for guidelines.
- Check any other comments are formatted correctly - see here for guidelines.
- Check Whitespace is being used correctly, this includes indentations and line breaks - see here for guidelines.
- Check the formatting of rulesets, properties and media queries are correct - see here for guidelines.
- As mentioned above, check existing RTL styles are formatted correctly - see here for guidelines.
Remaining tasks
- Review the patch - code review and visual changes
Review instructions: - Create a node with as much WYSIWYG content and different fields as you can.
- Take before and after applied screenshots of the node at a few different screen widths to show nothing/something is broken on the frontend.
User interface changes
None, we are cleaning up CSS and markup in templates. The use of Bartik's UI and design will stay the same.
API changes
n/a
Beta phase evaluation
Issue category | Task because it is a code clean up overhaul of a theme. |
---|---|
Unfrozen changes | Unfrozen because it only refactors CSS and templates, no changes to UI or APIs |
Prioritized changes | The main goal of this issue is usability and performance. We want Bartik code to be up to date and something to be proud of. |
Disruption | No disruption it's only refactoring code not changing how to use the theme |
Comment | File | Size | Author |
---|---|---|---|
#165 | bartik-content-css-visual-testing.pdf | 13.74 MB | emma.maria |
#165 | Screen Shot 2015-08-10 at 11.25.57.png | 385.17 KB | emma.maria |
#165 | Screen Shot 2015-08-10 at 11.23.50.png | 391.38 KB | emma.maria |
#163 | clean_up_the_content_2398463-162.patch | 20.87 KB | emma.maria |
#159 | issue.png | 292.74 KB | prabhurajn654 |
Comments
Comment #1
DickJohnson CreditAttribution: DickJohnson commentedWhile working for #2398461: Clean up the "comments" component in Bartik I noticed that styles for unpublished comments where both in comments.css and content.css. So removed it from content.css.
A patch that is doing only that.
Comment #2
adci_contributor CreditAttribution: adci_contributor commentedJust to test request
Comment #3
adci_contributor CreditAttribution: adci_contributor commentedComment #4
emma.mariaComment #5
DickJohnson CreditAttribution: DickJohnson commentedTook a quick look inside of current content.css file. Content as a name is way too generic and the file should be splitted up.
On lines 19-44 we have stuff that is related to view mode teaser in all content types. I would split this one up to it's own file.
On lines 61-91 we have stuff that is related to taxonomy term reference field. I'd put it either to it's own file or then I'd create a new file for all field-related styles.
On lines 117-177 we have node-preview related stuff which should be in already existing component called node-preview. Didn't make a complete check but it looks like most of it already is there.
Rest of the stuff is mostly .node__meta, .node__links, .node--unpublished etc node-related so for that I might create one file called node.css.
I will take a look on template a bit later.
Comment #6
DickJohnson CreditAttribution: DickJohnson commented#content has mentions in 6 different css-files:
1. admin.css
.path-admin #content image
2. content.css
#content h2
3. form.css
#content h2.comment-form
4. layout.css
50 different mentions
5. maintenance-page.css
.maintenance-page #content .section
6. print.css
.one-sidebar #content
.two-sidebars #content
I'm not sure if these should be handled together with this issue, but those mostly look like things we should get rid of.
Comment #7
DickJohnson CreditAttribution: DickJohnson commentedBy looking to page.html.twig I think that
That area is the one we should be looking for in this issue?
Comment #8
emma.mariaJust adding this as it was raised in this issue #2064379: Remove ckeditor-iframe.css and load relevant Bartik CSS files for CKEditor's iframe mode.. I agree with Wim and #5 in this issue. We should rename content.css to node.css as there are so many node styles and make sure all of the unapplicable CSS is split out into the correct components.
Comment #9
DickJohnson CreditAttribution: DickJohnson commentedWorked a bit on this. Compared node-preview related stuff and it was already on node-preview.css, so I removed it. Changed the file name to node.css also.
Comment #10
tadityar CreditAttribution: tadityar commentedHello!
This is my input reg patch in #9. How about making the diff with
git diff -C --staged
after adding all files so that the history won't be lost?Comment #11
tadityar CreditAttribution: tadityar commentedComment #12
tadityar CreditAttribution: tadityar commentedOh yeah my comment in #10 is about the renaming of content.css to node.css
Comment #13
DickJohnson CreditAttribution: DickJohnson commentedWorked a bit on this.
1. Separated purely field-related stuff to field.css.
2. Moved ul.links to list.css.
3. Changed node__meta to node__metadata in node.html.twig and changed the css to match that change.
4. Changed h1-id to class and css to match that.
5. Added few comments to files.
Comment #15
Wim LeersInvalid library definition in core/themes/bartik/bartik.libraries.yml: Unexpected characters near ";" at line 16 (near "css/components/field.css: {};").
i.e. this. Should be an easy fix :)
Comment #16
DickJohnson CreditAttribution: DickJohnson commentedWhoops. Fixed it.
Comment #17
lauriiiComment #19
DickJohnson CreditAttribution: DickJohnson commentedContent.css has been changed since my last patch so it needs reroll.
Comment #20
lauriiipage.html.twig has changed a bit so needed to reroll it. Also compiled multiple class attributes into one.
Comment #21
LewisNyman@lauriii It looks like we're missing the new node.css and field.css files?
I would be careful about doing this just in Bartik. This should be consistent across of core
Comment #22
DickJohnson CreditAttribution: DickJohnson commented"node__meta" has one mention outside Bartik, it's in core/modules/node/templates/node.html.twig line 92
<footer class="node__meta">
.My current quess is that it's not consistent across of core, but no idea how to grep all possible options.
Comment #23
tadityar CreditAttribution: tadityar commentedbtw patch in #20 didn't move the content of
content.css
tonode.css
Comment #24
thamasComment #25
lauriiiRedid the patch with the files included
Comment #26
LewisNymanThis styling is still in node.css. Is this a node specific thing? Maybe we should change the class to node__links?
The content component should stay in the content.css file
We always add em to the end of line height units.
Can we not leave these kinds of comments in the code? Conversation happens in the issue queue.
If we specify the full-content view mode gets the increased font size, then we won't have to reset it for the teaser, right?
Why do we have two classes? I don't think we use both
I think this makes more sense as just page-title. No reason for it to sit inside of component, also we have two class attributes now, I wonder if we can remove title....
I mean the system node.html.twig file, that has node__meta so if we are going to change the class we should do it there as well. Probably in another issue
Comment #27
DickJohnson CreditAttribution: DickJohnson commentedNode--unpublished points to unpublished nodes. Unpublished points to unpublished comments. So that should probably go under comments component and I'd like to see it as node--comment__unpublished or something.
Comment #28
amolnw2778 CreditAttribution: amolnw2778 commentedConcatenated common font-family selectors in three CSS files - content.css, header.css, tabs.css
Comment #29
rteijeiro CreditAttribution: rteijeiro commentedNice job @Amol,
A few things to consider:
- Don't forget to change issue status to "Needs review" in order to let testbot to test your patch and let reviews know that you submitted it.
- Your patch doesn't include the changes in the patch in comment #25: https://www.drupal.org/node/2398463#comment-9544083
- Read carefully the comments #26 and #27 and fix those suggestions.
Comment #30
amolnw2778 CreditAttribution: amolnw2778 commentedUpdated patch with #25 merged!
Comment #31
rteijeiro CreditAttribution: rteijeiro commentedNice work, @Amol. There are still a few things to do:
- In
core/themes/bartik/css/components/content.css
file I can see a few errors reported by CSSLint. For example the use of IDs and overqualified elements.Please, check the errors in this list made by @lewisnyman: http://lewisnyman.co.uk/drupalcore-frontend-toolkit
Let me know if you need help.
Comment #32
DickJohnson CreditAttribution: DickJohnson commentedIt doesn't look like #25 + #28 to me. In #25 there's a file called field.css, things done in page.html.twig and bartik.libraries.yml.
Comment #34
amolnw2778 CreditAttribution: amolnw2778 commentedUpdated patch considering comments #31, #32
Comment #36
amolnw2778 CreditAttribution: amolnw2778 commentedUpdated patch with minor correction. Hoping this will pass the test!
Comment #40
tadityar CreditAttribution: tadityar commented@amolnw2778 are you sure you are working with the latest updates of Drupal?
The patch can't be applied because things have changed since the version of Drupal you're using.
Please
git reset HEAD --hard
andgit pull origin 8.0.x
before making a patch :)Comment #41
amolnw2778 CreditAttribution: amolnw2778 commented+updates
Comment #42
amolnw2778 CreditAttribution: amolnw2778 commentedignore #41. Needs review
Comment #44
amolnw2778 CreditAttribution: amolnw2778 commentedFixes
Comment #46
tadityar CreditAttribution: tadityar commented@amolnw2778 When applying the patch the error lies in content.css, which means your content.css file is not up to date. It's also nice to post an interdiff after each change, see interdiff.
Comment #47
amolnw2778 CreditAttribution: amolnw2778 commented+1 content.css error fixes? hope so!
Comment #48
tadityar CreditAttribution: tadityar commented@amolnw2778 Great Job! I'll leave the reviewing to others :)
Comment #49
amolnw2778 CreditAttribution: amolnw2778 commented@tadityar this worked for me
git fetch
thengit reset --hard origin/8.0.x
but not yours, sorry! And thanks for the help :)Comment #50
LewisNymanThanks for the patches here! I think there are a few changes that need to be closer aligned to our CSS coding standards
I think we want to keep the font family in each component, it's important to keep all the styling for, see #2398447: Remove the "typography" CSS file in Bartik
We want to keep the node---unpublished file, the unpublished class is the one that is too broad and we should remove that one
We don't usually use @group tags in CSS, can we use standards docblocks please?
I think we only need the one class here, as the focus/hover/active selectors will inherit from it'?
I'm confused, why are we removing the margin?
Comment #51
amolnw2778 CreditAttribution: amolnw2778 commentedConsidered @LewisNyman's points.
font-family
selectors. Gone through the link. But still recommends to use global font-family at one place and let it inherit for all child selectors. Wonder if we could have one global stylesheet file, which takes care of all repetitive properties.unpublished
class and moved.node--unpublished
tonode.css
. How about moving all ^.node selectors tonode.css
?Comment #52
rteijeiro CreditAttribution: rteijeiro commentedNice job @Amol,
A few questions that maybe @lewisnyman can resolve:
I think we should remove this library dependency because the CSS file doesn't exist.
Same here.
We are not using
/* LTR */
comment to indicate left-to-right styling anymore?It still needs some work. You can see in the following screenshots that sidebar first blocks disappear and preview page is different (check preview button in the top):
Node BEFORE
Node AFTER
Preview BEFORE
Preview AFTER
Comment #53
emma.mariaComment #54
DickJohnson CreditAttribution: DickJohnson commentedOriginal idea was to get rid of content.css and replace it with node.css and field.css. Content as name is way too generic and its not a component.
Comment #55
LewisNymanOk, but this is undoing the work that we have just done in #2398447: Remove the "typography" CSS file in Bartik right? We are not in a situation where we can set one global font-family and let it inherit for everything, because we use two different font families for two different situations.
It's important that we try and make decisions that are inline with our CSS coding standards to keep our CSS consistent and predictable.
Comment #56
LewisNymanWe need a full stop at the end of this comment.
There are a few changes to other files that are not within the scope of this issue. Please try and keep all changes within the scope of this issue.
Comment #57
amolnw2778 CreditAttribution: amolnw2778 commented@Ruben #52:
node.css
,field.css
not exist, but has in the previous patch flow, hence included. @LewisNyman your call.@LewisNyman #56
node.css
due to patch flow.Comment #58
DickJohnson CreditAttribution: DickJohnson commentedIn @lauriiis patch #25 field.css had all field-spesific styles.
Comment #59
LewisNymanComment #60
LewisNymanThanks for the progress, we need to add the node.css and field.css files to the next patch.
This needs to be changed to a class not an ID.
We are setting the gradients on background color when they need to be set on background image, see the MDN documentation:
Writing these values in separate properties is less efficient then the previous implementation, we don't need to declare them separate because this is the first time we are setting these properties.
We don't need to add the rtl selectors here because they are inherited anyway.
This is not an LTR specific property so we don't need to add the comment here
We don't need the rtl selector here
Comment #61
amolnw2778 CreditAttribution: amolnw2778 commentedUpdated patch
Comment #62
rteijeiro CreditAttribution: rteijeiro commentedI think it still needs some work. There are things that change a little, like authoring font size (author and date) and the position of the body. Check screenshots:
Preview BEFORE
Preview AFTER
Node BEFORE
Node AFTER
Comment #63
amolnw2778 CreditAttribution: amolnw2778 commentedQuestion to experts!
I have included
node.css
inbartik.libraries.yml
filecss/components/node.css
, but to surprise,node.css
was not loaded on the page when I tested it in browser. See screenshotAnd that is why the CSS styles moved from content.css to
node.css
, is not getting applied! @Ruben has mentioned it as UI issue, which I doubt.So two questions:
node.css
is not getting loaded?node.css
(at for testing)?Comment #64
amolnw2778 CreditAttribution: amolnw2778 commentedFollowing CSS files included in header in 'View Source'
Comment #65
rteijeiro CreditAttribution: rteijeiro commented@amolnw2778, your patch doesn't include node.css file. I can't find it in
core/themes/bartik/css/components/node.css
Check patch in comment #25 and include node.css file and corrections pointed in comment #26, please.
Comment #66
amolnw2778 CreditAttribution: amolnw2778 commentedUpdated patch with
node.css
includedComment #67
rteijeiro CreditAttribution: rteijeiro commentedNow it's fixed. Thanks @Amol! Nice work!!
Node
Preview
Comment #68
alexpottWhere is field.css
Should this be here on in node - I would have thought node.
No new line at end of file.
Comment #69
tadityar CreditAttribution: tadityar commentedTrying to fix issues mentioned in #68.
Comment #70
emma.mariaReviewing the latest patch I have noticed that the most recent patches seem to contain very different content to the patch back in #25. The most recent patch also includes brand new CSS that did not exist originally in Bartik and has moved away from the great progress in #25. It has become very difficult to review with the variety of changes plus no interdiff files to accompany the patches.
As we have lost track of things I suggest we start over by creating a patch using the patch in #25 and adding the improvements suggested in #26. Then we can move on from there.
Here are instructions on how to apply the last patch someone worked on: Applying patches
How to create an interdiff to show the changes between the last patch and the patch you created: Interdiff instructions
Comment #71
DickJohnson CreditAttribution: DickJohnson commentedRerolled to #25 as @emma.maria said in #70. Starting to work on #26 and forward now.
Interdiff was not possible.
Comment #72
DickJohnson CreditAttribution: DickJohnson commentedFixed most of the issues from #72. Still continuing with this one.
Comment #73
DickJohnson CreditAttribution: DickJohnson commentedFixed em and page-title things from #26. Now this is really in need of review.
Comment #74
LewisNymanLooking good!
We need a blank line before a comment
Can we remove the .content class from this file and have this styling in content?
This feels like it should go inside the content file?
We also need a blank line before here
Also here
Comment #75
DickJohnson CreditAttribution: DickJohnson commentedFixed #74.1-#74.5.
Comment #76
thamasAfter applying the patch the layout is broken. Code review later.
Before:
After:
Comment #77
thamasClass is declared two times, it does not seem to work.
Comment #78
thamasThe goal of the layout rules defined here (and before and after) is to affect the
main
html element in our page template which have acontent
class (page.html.twig
, line 127).However some levels deeper we have a
div
which also has thecontent
class (defined byblock.html.twig
, line 52). This exact code shifts the content to the right which can be seen on my screenshot above, but in other situations (left sidebar etc) the same class can generate further problems.Comment #79
LewisNyman@thamas Ah yes, that is true, I think .content is too generic to be a reliable class, my suggestion is .main-content. Hopefully we will also fix the block classes soon as well to .block__content
Comment #80
thamas@LevisNyman So the next patch should change the class of the
main
element to.main-content
but changing the class of theblock
will be the subject of an other issue. Am I right?Comment #81
thamasIt is also the
.content
class ofdiv
what breaks the layout of the sidebar blocks. (See screenshot in #76.)It worth to mention that there is a
.sidebar .block .content
selector at line 20 insidebar.css
so we will lost its formatting when rename the block's class toblock__content
!Comment #82
thamascontent.css is removed while it has styling??
Comment #83
thamasFixed the issues mentioned in previous comments. Also set back the main content anchor class to id whicgh had been changed by accident.
(We still have several id-s in page.html.twig. Is this the subject of an other issue?)
(I missed the patch number, sorry.)
Comment #84
DickJohnson CreditAttribution: DickJohnson commentedWe have different clean up issues for every component. So yes, we will get rid of all ID's in page.html.twig, but it's going to take some time.
Comment #85
LewisNymanYep that makes sense, let's try and fix everything in content and avoid changing things in other files that we don't have to to fix content
Comment #86
LewisNymanRerolled
Comment #87
LewisNymanWe are almost there!
Can we move the page-title component into its own file?
Is it possible to use the .content class here as a wrapper instead of region-content? Then all the classes would match the file name
We've changed the class from content to main-content now right? There are still a lot of instances of the .content class, also the name of the files needs to be main-content.css instead of content.css
Comment #88
Maninders CreditAttribution: Maninders commented#87 I worked on the 3rd step of that. I am not able to understand 1st and 2nd point. Please elaborate that.
Comment #89
Maninders CreditAttribution: Maninders commented#87 I worked on the 3rd step of that. I am not able to understand 1st and 2nd point. Please elaborate that.
Comment #91
LewisNyman@Maninders Looks like your patch is missing a lot of changes from the previous patch?
Comment #92
thamas@LewisNyman So I will continue from #86 and #87
Comment #93
thamasRerolled. Currently applies to the latest HEAD.
Still needs to work in things mentioned in #87
Comment #94
thamasMade the changes mentioned in #87.
Also removed duplicated code from (main-)content.css which was added in the last reroll and moved new code which was added since the #87 into their own components.
Comment #95
LewisNymanThe only code improvement I could find was:
If this is only for the block component then we should add the
.block
class to the selector so it can't affect anything elseComment #96
thamasMade the recommended change from #95.
Also looked for selectors which was not changed from .content to .main-content. Found some and also fixed some code comment issues of those files.
Cleaning up content.css now seems to be ready (as we are left with only some lines) but I think we need follow up issues to clean up / refactor a lot of other (new) components.
Comment #97
thamas.comment-form class does not exist anymore. Comment form's title is styled in comments.css.
These rules does not apply but it is good, because it would be a broken layout. I do not know the motivation of these rules but it seems to me that they could be removed.
Comment #100
LewisNyman@thamas Ok thanks, I think a screenshot of the maintenance page would be useful to show we haven't broken anything?
Also tagging for visual regression testing.
We should avoid making changes to files outside the scope of this issue so we don't overlap with other issues, can you remove these comment changes please?
Comment #101
LewisNymanSetting to needs work toe revert the comment changes about. Feels like it's a novice issue
Comment #102
majmunbog CreditAttribution: majmunbog commentedFinished all the changes from the comment #100
Maintenance page looks the same as before the patch.
Comment #104
LewisNyman@trboslav I think there is a another patch mixed in with the patch you posted?
Comment #105
thamas@trboslav thanks for your work! Something must have gone wrong because there is a lot of "noise" (unneeded code at the beginning of the patch). We can fix it in the next step if you have more energy to work on it… :)
For anyone who feels that the screenshots shows that the two states are different: they are not, only the size of the two picture differs.
Comment #106
thamasAs we keep the original comment style of the forms.css file (in this issue) these empty lines should not be removed I think.
Comment #107
pguillard CreditAttribution: pguillard commentedRerolled : I took #96 and applied what is suggested at #100 and #106
Maintenance page after patch :
Comment #108
rteijeiro CreditAttribution: rteijeiro commentedLet's test it!
Comment #109
thamas@pguillard thanks! Patch applies flawlessly.
Lewis asked in #100 to keep this original comments. I do not know if it is really important.
What do you think Lewis?
Comment #110
thamasSo it is (almost) OK. How should it be reviewed (beyond checking the code)? Summary talks about screenshots but we could say what pages / content should be in screenshots.
Should / can we list the tasks of reviewing (by editing the summary)?
Comment #111
thamasComment #112
ti2m CreditAttribution: ti2m commentedI checked the patch in #107 with siteeffect and I couldn't find any regressions.
Comment #113
pguillard CreditAttribution: pguillard commentedCool. Maybe RTBC ?
Comment #114
LewisNymanGreat, thanks! Looks like it's ready to go. Good work everyone
Comment #115
emma.mariaThanks, before we set this to RTBC we should test the patch with examples of user generated content. This issue is for the content component so we should thoroughly test this and then post some screenshots to the issue.
Review instructions:
- Create a node with as much WYSIWYG content and different fields as you can.
- Take before and after applied screenshots of the node at a few different screen widths.
Comment #116
thamasComment #117
emma.mariaComment #118
emma.mariaTo review this issue I added dummy content to every block and also created 2 nodes and a view of the nodes.
I found a ton of visual regressions. I tested many URLs at 3 screen widths detailed in the test results and compared Bartik before and after applying the patch.
Attached is a pdf file containing all of the fails from the tool I used. I am going to go through and highlight the sections that are a problem in a follow up comment.
Comment #119
thamas@emma.maria Could you share the backtopjs.json you used for this test? Thanks!
Comment #120
emma.maria@thamas
Sure I've added it here. Also for my list of tests you need to create node/1 and node/2 content in Drupal, and a view of articles called article-view to get all results.
Also do not forget to enable all permissions in Drupal as the library tests as a logged out user.
Comment #121
thamasThanks @emma.maria! I create a similar test and will fix the problems.
Comment #122
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commented@thamas, are you still working on this issue? I got asked to look at this issue at the devdays as well, and i also have a solution. Thnx!
Comment #123
emma.mariaI am un-assigning, I have tried contacting @thamas through IRC and the issue and I have spoken to @haasontwerp who has a patch ready to contribute to this issue.
Comment #124
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedSo biggest problem was that node.css wasn't loading. After a clearing my cache, it did load and there were only a few minor visual differences, which i fixed.
Comment #125
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedComment #126
thamas@haasontwerp @emma.maria Sorry for being unavailable, I was on my way to home in the last days.
Comment #127
thamasPatch does not apply to the latest head… :(
Comment #128
tadityar CreditAttribution: tadityar commentedHope I rerolled it rightly.
Comment #129
thamasPatch applies to the latest head but there are visual regressions. (See the attached pdf.)
Comment #130
thamasNew patch. Applies to the latest head. No visual regressions. Comments later. :)
Comment #131
LewisNymanGreat work! I found only a few things to fix.
The @todo comments don't usually break the line, also this comment should have a full stop at the end
This is adding a footer.css file that doesn't exist, the file is called site-footer.css now, so we can just delete this line
This patch is adding all the CSS back into the layout.css file that was removed in #2398451: Clean up "layout" CSS in Bartik. I think we should be able to remove all of this CSS, let's just make sure that the main-content selectors are replicated in main-content.css
Comment #132
Manjit.SinghChanges as suggested in #131
Also @lewis
is already in header.css file. you can find it as
Comment #133
thamasCheck the patch.
Comment #136
LewisNymanWhen the length of this line goes over 80 characters, then we have to break the line. We should break to a new line before ".comment--unpublished"
We are still adding loads of CSS into layout.css that we removed before, we need to remove all these lines.
Comment #137
bixgomez CreditAttribution: bixgomez as a volunteer commentedComment #138
bixgomez CreditAttribution: bixgomez at CivicActions commentedSplit long (80+ char) comment line in comments.css file into two lines.
Modified layout.css file to eliminate class selectors that have previously been moved to other css files.
Comment #139
bixgomez CreditAttribution: bixgomez at CivicActions commentedChanging status to "needs review"
Comment #140
LewisNymanGreat, thank you!
Comment #143
lauriiire-rtbc
Comment #145
lauriiihttps://www.drupal.org/patch/reroll
Comment #146
Manjit.SinghComment #147
lauriiiComment #148
bill richardson CreditAttribution: bill richardson as a volunteer commentedpatch 146 breaks layout -- no sidebars,etc . Patch 132 looks a good candidate to work forward from.
Comment #149
emma.maria@Manjit.Singh Please in future can you include an interdiff file with every patch you create so we can see the changes you made.
I can not tell if the patch in #146, #138 or #132 introduces code that causes visual regressions as no one tested for this and we do not have any interdiffs.
The nicest way to handle this problem would probably be take the patch in #130 that was confirmed as not visually broken and apply the improvements stated in #131 and #136.
Comment #150
lauriiiemma.maria: interdiffs are not possible for rerolls :)
Comment #151
emma.maria@lauriii Ack I didn't see that. I was so caught up in looking inside the patches to see where things broke.
@Manjit.Singh Apologies :)
Comment #152
Manjit.Singh@emma no issues.
Comment #153
rachel_norfolkignore me - accidentally pressed submit on the wrong tab!!!
(hides in corner for a bit)
Comment #154
LewisNymanIt looks like there was some styling that was not moved from content.css into another CSS file. It would be worth someone going through each line that was removed to make sure that it was moved somewhere else.
Comment #155
emma.maria#2031883: Markup for: comment.html.twig being committed caused the patch #154 to need a reroll.
Here's the reroll.
Comment #156
emma.mariaI can confirm that there are still visual problems with the latest patch. I will try and debug them. Otherwise I might step back through patches to see where styles fell out.
Comment #157
rachel_norfolkWanted to take a look at this but first I seem to need to re-roll again...
Comment #158
nlisgo CreditAttribution: nlisgo commentedTrigger testbot
Comment #159
prabhurajn654 CreditAttribution: prabhurajn654 commented@emma.maria i am not able to reproduce issue which you shown via screenshots in #156. Can you please try to clear a cache.
when i cleared my cache its something looks like this. please check below screenshot
Comment #160
rachel_norfolk@prabhurajn654 - can you post a screenshot of what that same content looks like running on current 8.0.x branch, please? Would be good to see you comparison.
Also, which browser(s)?
Thanks
Comment #161
emma.mariaSorry with all the things being committed to Bartik recently this needs another reroll.
Comment #162
rachel_norfolkHa ha - after this week, I'm going to change my d.o username to re-roll-rachel!
Comment #163
emma.mariaItching to visually test the patch so I quickly rerolled.
Comment #164
nlisgo CreditAttribution: nlisgo commented@emma.maria we are trying to keep with you! :)
Comment #165
emma.mariaI have no idea how I saw the bugs in #156 but for this issue you definitely have to make sure you clear the caches.
Thank you @LewisNyman for finding the things that had fallen out of the patch in #154.
I tested the reroll in #163 and I found 0 visual regressions. I took note of the files affected in the patch and added dummy content of those component. I also tested one sidebar and two sidebars because of the previous layout issues due to things falling out the patch.
Here are some screenshots for good measure.
Also attached is a whopping big visual regression report that shows no differences.
Setting this to RTBC.
Comment #166
alexpottCommitted d5669ee and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #168
thamasYeah! :)
Comment #169
emma.mariaHooray! Thanks all!
Comment #170
Wim LeersThis caused a 404 for CKEditor iframe instances. Filed an issue + patch with the (very simple!) solution: #2552175: Follow-up for #2398463: Bartik is telling CKEditor to load a CSS file that no longer exists.
Comment #173
Maninders CreditAttribution: Maninders commentedComment #174
Maninders CreditAttribution: Maninders commentedComment #175
Maninders CreditAttribution: Maninders at Srijan | A Material+ Company commentedComment #176
Maninders CreditAttribution: Maninders at Srijan | A Material+ Company commented