Problem/Motivation
The node module has 3 different css files (node.admin.css, node.module.css, node.preview.css) that are all related to admin tasks.
Proposed resolution
Merge the 3 files into node.admin.css and remove any duplicated code.
Remaining tasks
1. Resolve questions raised in #35 @idbr
The issue summary indicates all 3 nodes.*.css files are merged into a single node.admin.css, but the patch leaves node.module.css. Is this on purpose?
node.admin.theme.css contains box-sizing, float, width attributes that deliver layout. This seems inconsistent with the SMACCS policy. Can you include a comment why these attributes are in a .theme file instead of a .module file?
User interface changes
Nil.
API changes
Nil.
Beta phase evaluation
Issue category | Task because code cleanup |
---|---|
Issue priority | Not critical because code cleanup | Unfrozen changes | Unfrozen because it only changes CSS |
Comment | File | Size | Author |
---|---|---|---|
#81 | interdiff_80-81.txt | 820 bytes | ravi.shankar |
#81 | 2421365-81.patch | 4.07 KB | ravi.shankar |
#80 | interdiff_78-80.txt | 1.02 KB | ravi.shankar |
#80 | 2421365-80.patch | 4.35 KB | ravi.shankar |
#78 | merge_node_css_files-2421365-78.patch | 4.59 KB | ameymudras |
Issue fork drupal-2421365
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
dmitrii CreditAttribution: dmitrii commentedLibrary drupal.node.admin (node.admin.css) - contains theme css, used for overview table of older revisions of a node.
Libraries form, drupal.node (node.module.css) - contains layout css, used for node form.
Library drupal.node.preview (node.preview.css) - contains theme css, used for single node in preview.
It look logical to keep all three files.
Comment #2
mortendk CreditAttribution: mortendk commentedFollowing the css code standards seperate the css files into 3 types:
Module: bare essentials, to make the module work
Admin: css used for admin part
Theme: visual enhanchemenrs for theme
[#1887922]
node form is admin, preview is admin -> all goes into node.admin.css
Comment #3
LewisNyman CreditAttribution: LewisNyman commentedIt seems like the layout this code is duplicated in node.module.css? We could remove a lot of code from there as well.
Comment #4
tadityar CreditAttribution: tadityar commentedThe Proposed resolution in the IS is to merge all 3 of them. So I removed node.module.css instead.
Comment #5
tadityar CreditAttribution: tadityar commented... Ignore #4
Comment #6
LewisNyman CreditAttribution: LewisNyman commentedThanks! We need screenshots in Stark/Seven/Bartik to show everything still looks right, as we need a screenshot of Stark without any of the .theme.css styling to show that it is still functional.
Comment #7
mortendk CreditAttribution: mortendk commentedComment #8
mortendk CreditAttribution: mortendk commentedfound an issues the library file wasnt changed to only call node.admin.css
uploaded new patch that adds the node.admin.css
I have changed the call to node.admin.css that but not changed the different libraries that are added so it needs a bit more work in node.libraries.yml
Comment #9
LewisNyman CreditAttribution: LewisNyman commentedNow we have four libraries that all call the same one file, maybe we can merge them together into one library?
Comment #10
mortendk CreditAttribution: mortendk commentedyup thats my next step - actually i do think we should have preview to be node.theme.css as its only colors, so a seperation would be in order.
Comment #11
mortendk CreditAttribution: mortendk commentedComment #12
mortendk CreditAttribution: mortendk commentedComment #14
mortendk CreditAttribution: mortendk commentedscreenshots:
Before patch
After patch
No theme
Comment #15
mortendk CreditAttribution: mortendk commentedComment #17
mortendk CreditAttribution: mortendk commentedlets try this again
Comment #18
mortendk CreditAttribution: mortendk commentedComment #19
rteijeiro CreditAttribution: rteijeiro commentedSorry for the following nitpicks >:P
Maybe we should rephrase and fix typo in this comment. Also comments should start with capitals, end with a period and have an space between comment tags.
Comments should start with capitals and end with a period.
Comment #20
mortendk CreditAttribution: mortendk commentedComment #21
mortendk CreditAttribution: mortendk commentedComment #22
rteijeiro CreditAttribution: rteijeiro commentedFixed a missing period in a comment block and removed periods in inline comments. Not sure if they should have periods or not. I haven't found any info about it. Any ideas?
The rest of the patch seems to be ok and it works as expected. Check screenshots:
Preview BEFORE
Preview AFTER
Node BEFORE
Node AFTER
Comment #23
LewisNyman CreditAttribution: LewisNyman commented@rteijeiro Doxygen recommends full stops on the first line of a comment. I think the easiest rule to follow here is to put a full stop on every comment.
I've updated the CSS coding standards examples so they consistently use a full stop.
Comment #24
mortendk CreditAttribution: mortendk commentedadded the missing dots - patch uploaded & interdiff
Comment #25
mortendk CreditAttribution: mortendk commentedComment #26
LewisNyman CreditAttribution: LewisNyman commentedThanks for all the screenshots. It looks like we've fixed all the concerns here. I manually tested it again then just to make sure we didn't loose any styling. It looks good.
Comment #27
mortendk CreditAttribution: mortendk commentedcheers!
think this might be "patch of the year with most screenshots" ;)
Comment #28
alexpottThe new css has lots of additional css. Surprising.
Comment #29
mortendk CreditAttribution: mortendk commentedsomehow the diff didnt get the removal of
node.module.css
:(new patch added - with interdiff
Comment #30
joelpittetThis looks great, just a couple of questions:
Should this go above the @media query? Not sure the pattern we are using here again...
Shouldn't this be in a theme library category or something? Just guessing...
Comment #31
mortendk CreditAttribution: mortendk commented1. nope its only if they are related to each other
.node-preview-container .form-type-select
&.revision-current
isnt related2. afaik we don't use theme libraries for admin.theme.css ?
Comment #32
joelpittet2. I don't know the rules around *.theme.css and theme: in yaml Maybe there is a rule you or @LewisNyman can point me to?
Comment #33
mortendk CreditAttribution: mortendk commented@joel 2. yup it should be theme library sets it back to needs work
Comment #34
mortendk CreditAttribution: mortendk commentedfixed layout to theme in lib
Comment #35
idebr CreditAttribution: idebr commentedThere is still a lot of duplicated css between node.module.css and node.admin.css.
Comment #36
mortendk CreditAttribution: mortendk commentednode.module should only have "stuff thats absolutely nessesary" for the module to work. So it a themer kills every css file there is and leaves
*.module.css
sutff still works but looks like crap ;)yeah node.module.css should be killed in the diff, or did my git-fu fail me again
Comment #37
mortendk CreditAttribution: mortendk commentedComment #38
njbarrett CreditAttribution: njbarrett commentedI am at the drupal south sprint and going to update the issue summary
Comment #39
njbarrett CreditAttribution: njbarrett commentedI applied the patch in #34 and noticed that node.module.css still exists, this should be removed.
The only file that remains then is node.admin.theme.css, which seems inconsistent with the issue summary, should this be called node.admin.css ?
Comment #40
njbarrett CreditAttribution: njbarrett commentedComment #41
njbarrett CreditAttribution: njbarrett commentedComment #42
akalata CreditAttribution: akalata commentedFixing which CSS files are created/named/deleted.
Comment #43
LewisNyman CreditAttribution: LewisNyman at Wunder commentedIt looks like this introduces a problem with the node preview bar. Because the node preview page is is not an admin page, moving the CSS into the admin theme breaks the layout of the node preview bar. I think we should keep node preview in it's own file and just convert node.module.css to node.admin.css.
Before:
After:
Comment #44
akalata CreditAttribution: akalata commentedIn reading up on the docs to answer #43, I've realized that my change in #42 to answer #39 is actually incorrect -- the file should be named node.admin.theme.css (point 4 on [#2016305]).
Keeping node.preview.css breaks the naming conventions from what I could see. I think the correct answer would be to fix the preview page so that it loads the admin CSS.
Comment #45
LewisNyman CreditAttribution: LewisNyman at Wunder commentedI am worried that if we load all the admin CSS on a frontend page we might introduce loads of regressions. What if we put the preview CSS inside a node.theme.css file?
Comment #46
akalata CreditAttribution: akalata commentedThat sounds like a good compromise. I've basically renamed node.preview.css to node.theme.css, updated node.libraries.yml, and removed node.module.css that #34 missed.
Comment #47
akalata CreditAttribution: akalata commentedComment #48
LewisNyman CreditAttribution: LewisNyman at Wunder commentedOk, I've taken screenshots of the node preview bar in Bartik and the node edit form in Seven and Classy to show there are no UI changes and added them to the issue summary. Thanks.
Comment #49
alexpottCreating a node is not an necessarily an administrator task - i'm not sure that node.admin.css is the correct name.
Comment #50
akalata CreditAttribution: akalata commentedI don't think the designation is role based, it's mode/screen based. The node module's primary function is to create, manage, and display nodes. Following the guidelines outlined at https://www.drupal.org/node/2016305:
Comment #51
alexpott@akalata yep I can buy that.
Comment #52
alexpottSo whilst I buy it - I don't completely.
The preview code is in node.theme.css after this patch. Because it appears in the main theme - but the same can be true of the node edit css.
Comment #53
mortendk CreditAttribution: mortendk commented@alex yes it can sometimes be true for the editing of a node. but the normality (if i can use that word in drupal) is that we dont use the main theme for editing, its more the exception :)
oooh my if we didnt have this fussy level between admin & edit & enduser & whathave we not ;)
Comment #54
catchIf we put the edit CSS in admin, we should put preview in too. Preview can only be got to from edit.
Comment #57
LewisNyman CreditAttribution: LewisNyman at Wunder commentedComment #58
googletorp CreditAttribution: googletorp as a volunteer commentedRerolled patch
Comment #70
quietone CreditAttribution: quietone at PreviousNext commentedFrom reading the IS, this is a task not a bug.
Comment #72
anoopsingh92Comment #73
anoopsingh92Comment #74
anoopsingh92#58 patch failed for me.
Comment #75
amin.ankitComment #76
amin.ankitComment #77
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #78
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedRe rolled the above patch for Drupal 9.4.x
Comment #79
smustgrave CreditAttribution: smustgrave at Mobomo commentedPatch failed. Don't see any answer for #35 in the issue summary.
Comment #80
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed Drupal CS issues of patch #78, still needs work for issue summary update.
Comment #81
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixing Drupal CS issues of patch #80.