Clean up locale CSS in line with our CSS standards

Added patch and screenshots.

Before

After

@rudraram:
Thanks for looking into this.

It seems like all the lint-errors would be fixed by patch #1.

Lint errors were:
[16:32:46] 12 errors found in /Applications/MAMP/htdocs/drupal8_local/core/modules/locale/css/locale.admin.css
[16:32:46] [L62:C1] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L65:C1] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L65:C33] Element (th.select-all) is overqualified, just use .select-all without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
[16:32:46] [L68:C1] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L71:C1] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L71:C1] Rule is empty. Rules without any properties specified should be removed. (empty-rules)
[16:32:46] [L73:C1] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L85:C1] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L90:C1] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L127:C3] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L130:C3] Don't use IDs in selectors. Selectors should not contain IDs. (ids)
[16:32:46] [L130:C35] Element (th.status) is overqualified, just use .status without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)

Some thoughts:
1) The current page breaks with me locally:

But this isn't because of the patch itself, it seems to be broken by a other previous commit.

2) The css will need some more refactoring:

.locale-translate-filter-form .form-item-langcode,
.locale-translate-filter-form .form-item-translation,
.locale-translate-filter-form .form-item-customized {
  float: left; /* LTR */
  margin-right: 1em; /* LTR */
  margin-bottom: 0;
  /**
   * In Opera 9, DOM elements with the property of "overflow: auto"
   * will partially hide its contents with unnecessary scrollbars when
   * its immediate child is floated without an explicit width set.
   */
  width: 15em;
}
[dir="rtl"] .locale-translate-filter-form .form-item-langcode,
[dir="rtl"] .locale-translate-filter-form .form-item-translation,
[dir="rtl"] .locale-translate-filter-form .form-item-customized {
  float: right;
  margin-left: 1em;
  margin-right: 0;
}
.locale-translate-filter-form .form-type-select select {
  width: 100%;
}

We shouldn(y be using 3 lines of selectors, instead, this should be something like:.locale-translate-filter-form .form-item

Also the overuse of comments I found very strange

@MathieuSpil I agree with you. I will work and re-factor the selectors.

@LewisNyman Any thoughts on the comments use in the file?

Changed ID's to classes, fixed CSS to follow coding standards.

Optimised selectors:
[dir="rtl"] .locale-translate-filter-form .form-item-langcode,
[dir="rtl"] .locale-translate-filter-form .form-item-translation,
[dir="rtl"] .locale-translate-filter-form .form-item-customized

Removed space.

Optimised selectors for:
.locale-translate-filter-form .form-item-langcode,
.locale-translate-filter-form .form-item-translation,
.locale-translate-filter-form .form-item-customized
[dir="rtl"] .locale-translate-filter-form .form-item-langcode,
[dir="rtl"] .locale-translate-filter-form .form-item-translation,
[dir="rtl"] .locale-translate-filter-form .form-item-customized

Changed ID's to classes.

Yes, I agree. That is also applying to this:

-[dir="rtl"] .locale-translate-filter-form .form-item-langcode,
-[dir="rtl"] .locale-translate-filter-form .form-item-translation,
-[dir="rtl"] .locale-translate-filter-form .form-item-customized {
+[dir="rtl"] .locale-translate-filter-form [class*="form-item"] {

As a summary I added the interdiff between #1 and #14.
The patches don't apply anymore, but next to that, I don't think the changes aren't needed started from #1?

Therefor I rerolled the patch from #1, Not sure why the first patch didn't apply anymore...

Patch from #16 was not applicable. Rerolled.

Patch from #18 applies cleanly for me and looks good. Note that on a fresh installation it is overridden by CSS in the "Stable" base theme.

Thanks for working on this! Assigning frontend RTBC issues to Cottser for possible review.

Looks like a solid improvement overall, thanks everyone.

When testing in RTL and reviewing the CSS I noticed that this report form has some RTL inconsistencies so I created a separate issue to discuss that. Potentially converting this to <details> is the best way to fix that.

For now can we please add /* LTR */ comments that are missing? https://www.drupal.org/node/1887862#rtl

1. .locale-translation-update__wrapper {
     background: transparent url(../../../misc/menu-collapsed.png) left .6em no-repeat;
     margin-left: -12px;
     padding-left: 12px;
   }
   .expanded .locale-translation-update__wrapper {
     background: transparent url(../../../misc/menu-expanded.png) left .6em no-repeat;
   }

   • Background positioning to the left - needs LTR comments.
   • The margin/padding I'm fairly sure also needs LTR comments.

2. .locale-translation-update__details li {
     margin: 0 0 0.25em 1.5em;
     padding: 0;
   }
   

   Left margin - should have an LTR comment.

Forgot to link this up to the related issue I created.

New patch with /* LTR */ comments added per request. This is my first contrib and I'm working with cwells and starshaped at #d4dBoston!

@andrea.piernock an interdiff would be useful to review this. Here are some instructions on how to do it :)

I went ahead and created an interdiff. This is my first time ever creating one so be gentle :) Let me know if this looks good to review.

@darketaine: thanks for the link and the info on adding an interdiff! and thanks to @starshaped for adding the interdiff :)

@starshaped: Thanks for providing the interdiff. I hope my criticism is constructive and gentle.

It looks as though you uploaded another copy of the patch from #25 when you added the interdiff. Not a big problem, but it does mean

1. The testbot does a little extra work re-testing the same patch.
2. If the maintainer agrees to commit the patch, then (s)he has to spend a minute or two deciding which link to download.

In the interest of avoiding #2, I have checked: the patches attached to #25 and #27 are identical.

Looking at the issue summary, there is one remaining task:

• Run CSSlint against the new CSS

Updating the list of remaining tasks in the issue summary.

first time doing a CSSLint task :)

I ran the patch through a CSS linter and got 1 warning:
At line 104, character 3: The property -epub-hyphens is compatible with -moz-hyphens and should be included as well.

I'm not really sure how to proceed..

Resolved the CSSLint warning reported in #31.

I'm looking at this issue @DrupalCon Dublin

Looking at the interdiff from #32, it looks as though it might be unrelated to this issue. If so, we should probably use the earlier patch. Has anyone checked whether there is already an issue to deal with -epub-hyphens?

I went ahead and removed -epub-hyphens, and checked to see if an issue for this exists already. One does not, so should I create an issue for this?

Also, since the only CSS Lint error pertains to -epub-hyphens and this is an issue that will be taken care of in a yet to be created issue, does this mean the CSS Lint task here is completed?

Opened a follow-up issue to add -epub-hyphens to locale's css: #2940160: Add -epub-hyphens to locale's css

Re-rolled patch to fix apply failures.

In the patch from #16, I see

-#locale-translation-status-form .label {
+.locale-translation-status-form .label {
   color: #1d1d1d;
   font-size: 1.15em;
   font-weight: bold;
 }

That is a +1-1 change with a few lines of context. In the later patches, I see

-#locale-translation-status-form .description {
+.locale-translation-status-form .label {
+  color: #1d1d1d;
+  font-size: 1.15em;
+  font-weight: bold;
+}
+.locale-translation-status-form .description {

That is a +6-1 change. In other words, it introduces three new rules for the selector .locale-translation-status-form .label. I think this is accidentally reverting #2573139: Followup: Remove unneeded label class from admin/reports/translations.

After reviewing #2573139: Followup: Remove unneeded label class from admin/reports/translations, it does appear that .locale-translation-status-form .label is being re-added when it shouldn't be. Updated the patch by taking out this selector.

Sorry I did not notice this earlier, but https://www.drupal.org/node/1887862#rtl (already mentioned by @Cottser in Comment #23) says that, in addition to the /* LTR */ comment, we should add the corresponding [dir="rtl"] variant of the rules. This has already been done for existing LTR comments, but I think we have to do it for the ones we add in this issue.

I would like to see the results of the linter.

This patch is built on top of #43's patch. I've added the [dir="rtl"] to the rules with the comments next to them and i've added -epub-hyphens as it still appeared as an error from CSSLint. I'll attach the rest of the CSSLint output to this too.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I have refactored the CSS file.
1. Replaced all the ID selectors with class selectors.
2. margin-left, right properties replaced with margin-inline-start, end so it can be used in LTR & RTL.

Please review.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed CCF and attached interdiff for same.

There are few instances of hardcoded colors. Should those be variables or least the 6 character colors?

Updated #61 according to #62

Added color variables and added interdiff with #61, for same. please review

Thanks @Gauravv!

Just adding a space because I keep thinking this is about inline styles.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolled the patch.

Reroll seems fine.

Worked on fail test at #69 and found that the test are passing on local so rerun the test again on #69 and test are passing now, So as per the #70 moving it to RTBC.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

#69 still applied cleanly so this could of been put back into RTBC.

Manually tested this and all looks fine, but there is one LTR comment that I think is out of place, and we should fix it while we're here.

Rebased the MR for pipeline break.

Feedback appears to be addressee

It'd be great to do #3312966: Enforce the use of CSS Logical Properties in core first as that would enforce quite a few of the changes.

Postponing on #3312966: Enforce the use of CSS Logical Properties in core per #85.

Also saving issue credits, which took a long time since this has been reviewed by a lot of different people over the years. I made an educated guess on which "cwells" was mentoring @reedcodes in Boston. :)