Problem/Motivation
See: #1995272: [Meta] Refactor module CSS files inline with our CSS standards
Proposed resolution
The page in question can be seen on admin/reports/translations after language modules are enabled.
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
Run CSSlint against the new CSS
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|
Issue fork drupal-2485497
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
rudraram CreditAttribution: rudraram at Axelerant commentedAdded patch and screenshots.
Before
After
Comment #2
rudraram CreditAttribution: rudraram at Axelerant commentedComment #3
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented@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:
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
Comment #4
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedComment #5
rudraram CreditAttribution: rudraram at Axelerant commented@MathieuSpil I agree with you. I will work and re-factor the selectors.
@LewisNyman Any thoughts on the comments use in the file?
Comment #6
LewisNymanI think the comment is ok. If we need a multi-line comment then I think that's fine.
Comment #7
SixZeroNine CreditAttribution: SixZeroNine at Develomon for FFW commentedChanged ID's to classes, fixed CSS to follow coding standards.
Comment #8
SixZeroNine CreditAttribution: SixZeroNine at Develomon for FFW commentedOptimised 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
Comment #9
SixZeroNine CreditAttribution: SixZeroNine at Develomon for FFW commentedRemoved space.
Comment #10
SixZeroNine CreditAttribution: SixZeroNine at Develomon for FFW commentedOptimised 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.
Comment #11
SixZeroNine CreditAttribution: SixZeroNine at Develomon for FFW commentedComment #12
LewisNymanThanks, I think that this selector is too broad. It could affect other form item elements by mistake. I think for now maybe we are better just using the classes we have right now.
Comment #13
SixZeroNine CreditAttribution: SixZeroNine at Develomon for FFW commentedComment #14
SixZeroNine CreditAttribution: SixZeroNine at Develomon for FFW commentedYes, I agree. That is also applying to this:
Comment #15
SixZeroNine CreditAttribution: SixZeroNine at Develomon for FFW commentedComment #16
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedAs 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...
Comment #18
AjitSPatch from #16 was not applicable. Rerolled.
Comment #19
Amerie CreditAttribution: Amerie commentedPatch 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.
Comment #21
markdorisonComment #22
xjmThanks for working on this! Assigning frontend RTBC issues to Cottser for possible review.
Comment #23
star-szrLooks 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#rtlLeft margin - should have an LTR comment.
Comment #24
star-szrForgot to link this up to the related issue I created.
Comment #25
reedcodes CreditAttribution: reedcodes as a volunteer commentedNew patch with /* LTR */ comments added per request. This is my first contrib and I'm working with cwells and starshaped at #d4dBoston!
Comment #26
darketaine CreditAttribution: darketaine at Pixual commented@andrea.piernock an interdiff would be useful to review this. Here are some instructions on how to do it :)
Comment #27
starshapedI 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.
Comment #28
reedcodes CreditAttribution: reedcodes as a volunteer commented@darketaine: thanks for the link and the info on adding an interdiff! and thanks to @starshaped for adding the interdiff :)
Comment #29
benjifisher@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
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:
Comment #30
benjifisherUpdating the list of remaining tasks in the issue summary.
Comment #31
sugaroverflow CreditAttribution: sugaroverflow commentedfirst 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..
Comment #32
markdorisonResolved the CSSLint warning reported in #31.
Comment #34
anouschka42 CreditAttribution: anouschka42 commentedI'm looking at this issue @DrupalCon Dublin
Comment #35
benjifisherLooking 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
?Comment #38
starshapedI 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?Comment #40
starshapedOpened a follow-up issue to add -epub-hyphens to locale's css: #2940160: Add -epub-hyphens to locale's css
Comment #41
starshapedRe-rolled patch to fix apply failures.
Comment #42
benjifisherIn the patch from #16, I see
That is a +1-1 change with a few lines of context. In the later patches, I see
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.Comment #43
starshapedAfter 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.Comment #44
starshapedComment #45
benjifisherSorry 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.
Comment #46
rakesh.gectcrComment #47
ryanhayes CreditAttribution: ryanhayes as a volunteer commentedThis 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.
Comment #48
ryanhayes CreditAttribution: ryanhayes as a volunteer commentedComment #58
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #59
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedI have refactored the CSS file.
1. Replaced all the ID selectors with class selectors.
2.
margin-left, right
properties replaced withmargin-inline-start, end
so it can be used in LTR & RTL.Please review.
Comment #60
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #61
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedFixed CCF and attached interdiff for same.
Comment #62
smustgrave CreditAttribution: smustgrave at Mobomo commentedThere are few instances of hardcoded colors. Should those be variables or least the 6 character colors?
Comment #63
Rishabh Vishwakarma CreditAttribution: Rishabh Vishwakarma at OpenSense Labs for DrupalFit commentedUpdated #61 according to #62
Comment #64
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedAdded color variables and added interdiff with #61, for same. please review
Comment #65
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks @Gauravv!
Comment #66
catchJust adding a space because I keep thinking this is about inline styles.
Comment #68
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #69
sagarchauhan CreditAttribution: sagarchauhan at Specbee for Drupal India Association commentedRerolled the patch.
Comment #70
smustgrave CreditAttribution: smustgrave at Mobomo commentedReroll seems fine.
Comment #72
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedWorked 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.
Comment #73
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.)
Comment #76
djsagar CreditAttribution: djsagar at OpenSense Labs commentedComment #77
smustgrave CreditAttribution: smustgrave at Mobomo commented#69 still applied cleanly so this could of been put back into RTBC.
Comment #78
longwaveManually 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.