Problem/Motivation

See: #1995272: [Meta] Refactor module CSS files inline with our CSS standards

Proposed resolution

Review the CSS against our standards, see:
http://drupal.org/node/1887918#best-practices
https://www.drupal.org/node/2408617
#1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core

Remaining tasks

Review current CSS
Write a patch to fix the suggestions
Run CSSlint against the new CSS

Testing steps

You have to enable the core-language modules
Add a language
Go to admin/config/regional/content-language

User interface changes

None

API changes

None

CommentFileSizeAuthor
#11 rtl-after-content-translation.png68.55 KBmanjit.singh
#11 rtl-before-content-translation.png69.83 KBmanjit.singh
#10 After.png22.73 KBrudraram
#10 Before.png23.72 KBrudraram
#7 interdiff-2485409-4-7.txt1.54 KBmanjit.singh
#7 content-translation-css-2485409-7.patch2.14 KBmanjit.singh
#4 content-translation-css-2485409-4.patch2.19 KBrudraram
#3 content-translation-css-2485409-3.patch2.23 KBrudraram
content_translation.admin_.css_.txt790 byteslewisnyman

Comments

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
lewisnyman’s picture

lewisnyman
he/him
commented
Related issues: +#2485439: Clean up language CSS inline with our CSS standards
rudraram’s picture

rudraram commented
Status: Active » Needs review
StatusFileSize
new content-translation-css-2485409-3.patch2.23 KB

Added class to the form and updated css.

rudraram’s picture

rudraram commented
StatusFileSize
new content-translation-css-2485409-4.patch2.19 KB

The above patch was wrong. Updated it.

The last submitted patch, 3: content-translation-css-2485409-3.patch, failed testing.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work 
+++ b/core/modules/content_translation/css/content_translation.admin.css
@@ -3,31 +3,31 @@
-#language-content-settings-form table .bundle {
+.language-content-settings-form table .bundle {
...
-#language-content-settings-form table .field {
+.language-content-settings-form table .field {
...
-[dir="rtl"] #language-content-settings-form table .field {
+[dir="rtl"] .language-content-settings-form table .field {
...
-#language-content-settings-form table .column {
+.language-content-settings-form table .column {
...
-[dir="rtl"] #language-content-settings-form table .column {
+[dir="rtl"] .language-content-settings-form table .column {
...
+.language-content-settings-form table .field label,
+.language-content-settings-form table .column label {
...
-#language-content-settings-form table .translatable {
+.language-content-settings-form table .translatable {
...
-#language-content-settings-form table .operations {
+.language-content-settings-form table .operations {

Can we remove the table element from these selectors?

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
StatusFileSize
new content-translation-css-2485409-7.patch2.14 KB
new interdiff-2485409-4-7.txt1.54 KB

removing table element as suggested by @lewis

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Status: Needs work » Needs review
lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: +Needs screenshots

Great, now we just need some before/after screenshots

rudraram’s picture

rudraram commented
StatusFileSize
new Before.png23.72 KB
new After.png22.73 KB

Adding screenshots

Before:
before

After:
before

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new rtl-before-content-translation.png69.83 KB
new rtl-after-content-translation.png68.55 KB

uploaded rtl screenshots as well ;)

content translation

content translation

seems like no regression issues.. :) changing status to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: content-translation-css-2485409-7.patch, failed testing.

Manjit.Singh queued 7: content-translation-css-2485409-7.patch for re-testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

Back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Isn't there a patch somewhere to change this to use the standard admin layout classes?

lewisnyman’s picture

lewisnyman
he/him
commented

@alexpott These width properties are for table cells instead of regular elements so we can't apply the reuseable classes to them. I'm not sure how we can make this CSS more generic, so for now the best we can do it remove the CSSLint errors.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 880007d and pushed to 8.0.x. Thanks!

CSS is not frozen in beta.

  • alexpott committed 880007d on 8.0.x 
    Issue #2485409 by rudraram, Manjit.Singh, LewisNyman: Clean up content...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.