Found some coding standards that needed to be fixed.

CommentFileSizeAuthor
#23 coding-standard-result.png212.88 KBNeslee Canil Pinto
#21 fix-css-coding-standard-3005007-21.patch15.92 KBNeslee Canil Pinto
#19 fix-css-coding-standard-3005007-19.patch16.01 KBNeslee Canil Pinto
#17 fix-css-coding-standard-3005007-17.patch16.01 KBNeslee Canil Pinto
#15 fix-css-coding-standard-3005007-15.patch16 KBNeslee Canil Pinto
#13 fix-css-coding-standard-3005007-13.patch16 KBNeslee Canil Pinto
#11 fix-css-coding-standard-3005007-11.patch16.01 KBNeslee Canil Pinto
#8 fix-css-coding-standard-3005007-8.patch15.87 KBNeslee Canil Pinto
#6 fix-css-coding-standard-standard-3005007-6.patch553 bytesNeslee Canil Pinto
#4 coding_standards-3005007-4.patch15.86 KBDeaOm
#2 coding_standards-3005007-2.patch62.93 KBDeaOm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DeaOm created an issue. See original summary.

DeaOm’s picture

DeaOm CreditAttribution: DeaOm at Agiledrop - Your Trusted Drupal Teammates commented
Assigned: DeaOm » Unassigned
Status: Active » Needs review
FileSize
coding_standards-3005007-2.patch62.93 KB

Status: Needs review » Needs work

The last submitted patch, 2: coding_standards-3005007-2.patch, failed testing. View results

DeaOm’s picture

DeaOm CreditAttribution: DeaOm at Agiledrop - Your Trusted Drupal Teammates commented
FileSize
coding_standards-3005007-4.patch15.86 KB

Fixed most of the CSS problems, the backgorund image one still remains, because it needs to be added as it's own class, so there is no repeating of the image, which is not allowed. But there was no twig template, so I hope everything still works after CSS changes. Needs review and work.

HongPong’s picture

HongPong CreditAttribution: HongPong at kor group commented
Title: Fix coding standards » Fix CSS coding standards

Missing semicolon here on the background attribute. Also I believe this should be a background-image attribute if there is no positioning or size included.

	+.admin-menu .dropdown .admin-menu-toolbar-category > active-trail {
   text-shadow: #333 0 1px 0;
-  background: url(toolbar.png) 0 0 repeat-x;
+  background: url(toolbar.png)
Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
fix-css-coding-standard-standard-3005007-6.patch553 bytes

Coding Standards added

Status: Needs review » Needs work

The last submitted patch, 6: fix-css-coding-standard-standard-3005007-6.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
FileSize
fix-css-coding-standard-3005007-8.patch15.87 KB

Updated Coding Standards

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: fix-css-coding-standard-3005007-8.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
fix-css-coding-standard-3005007-11.patch16.01 KB

Updated Coding Standard

Status: Needs review » Needs work

The last submitted patch, 11: fix-css-coding-standard-3005007-11.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
fix-css-coding-standard-3005007-13.patch16 KB

Updated Coding Standard

Status: Needs review » Needs work

The last submitted patch, 13: fix-css-coding-standard-3005007-13.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
fix-css-coding-standard-3005007-15.patch16 KB

Updated Coding Standard

Status: Needs review » Needs work

The last submitted patch, 15: fix-css-coding-standard-3005007-15.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
fix-css-coding-standard-3005007-17.patch16.01 KB

Updated Coding Standard

Status: Needs review » Needs work

The last submitted patch, 17: fix-css-coding-standard-3005007-17.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
fix-css-coding-standard-3005007-19.patch16.01 KB

Updated Coding Standard

Status: Needs review » Needs work

The last submitted patch, 19: fix-css-coding-standard-3005007-19.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
fix-css-coding-standard-3005007-21.patch15.92 KB

Updated Coding Standard

Status: Needs review » Needs work

The last submitted patch, 21: fix-css-coding-standard-3005007-21.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
coding-standard-result.png212.88 KB

Fixed all coding standard issues. Build Showing Test Failed

HongPong’s picture

HongPong CreditAttribution: HongPong at kor group commented

This cleanup looks good to me. @Neslee Canil Pinto keep in mind that "failed testing" results right now are to be expected since the test framework code has not been revised much if at all yet. The only problem you should be concerned about is if the patch doesn't apply, I believe. (also in most cases the test frameworks here are not aware of CSS in general)

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented

Ok @HongPong
So Further how should I proceed

HongPong’s picture

HongPong CreditAttribution: HongPong at kor group commented

These all appear to be the correct changes to me in this case. I have not tested this patch but it covers: replacing ID with class; capitalization; removing !important tags (yes!). This brings us much closer to correct CSS. So with the point we are at now, I think it should be committed, but we should get another opinion for more certainty.

Also in the future we need to be aware, as you covered in this patch, that there are multiple CSS files to keep track of on this module (we have seen other patches that did not cover some of them).

I think we could approve this and move onto #2490318: Administration Menu Javascript ESLint errors to cover the Javascript equivalent/similar code style issues. All of this helps because then we won't get tons of confusing code smell warnings, moving into the trickier issues later. Thanks again!!

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented

Hey HongKong
Can you commit this patch
https://www.drupal.org/project/admin_menu/issues/3005007#comment-12937138

HongPong’s picture

HongPong CreditAttribution: HongPong at kor group commented

@Neslee if I can get a little time I will test it today and if it's solid I will commit it.

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented

Ok @HongPong take your time👍

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
CreditAttribution: thalles at CI&T commented

Replacing # admin-menu by .admin-menu would not break any menu?

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor as a volunteer and at Drupal Ukraine Community, GOLEMS GABB for GOLEMS GABB, Drupal Ukraine Community commented
Status: Needs review » Needs work

Replacing # admin-menu by .admin-menu would not break any menu?

In my opinion, this should not be done.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
CreditAttribution: thalles at CI&T commented
Category: Task » Support request
Status: Needs work » Fixed

This module has been deprecated for Drupal 8.
For Drupal 8: the Admin Toolbar provides an experience similar to admin_menu with the core toolbar.

Thanks @all!

Status: Fixed » Closed (fixed)

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