Posted by andrewmacpherson on

The theme settings form has some redundant CSS dating from D6.

Logo image settings has a "theme-settings-bottom" class which was used to apply clear:both to the fieldset, to stop it floating against the Toggle display fieldset.

It's no longer needed since we took away the "Display post information on" fieldset in D7 and made the Toggle display fieldset full-width.

It's not causing any problems in D7 / D8 core, but it is causing an issue for one of my modules in contrib (see #1757690: Big white space before the widgets in the "Logo image settings" vertical tab).

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Unfrozen changes Unfrozen because it only removes unused CSS.
Files: 
CommentFileSizeAuthor
#15 drupal-remove_redundant_css_from_theme_settings_form-1759090-15.patch1.18 KBjjcarrion
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,820 pass(es).
[ View ]
#11 drupal-remove_redundant_css_from_theme_settings_form-1759090-11.patch1.24 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 63,143 pass(es).
[ View ]

Comments

andrewmacpherson’s picture

andrewmacpherson commented
Status:Active» Needs review
StatusFileSize
new themesettings-CSS-cleanup-D8-1759090-1.patch1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 40,711 pass(es).
[ View ]

Patch for D8

andrewmacpherson’s picture

andrewmacpherson commented
StatusFileSize
new themesettings-CSS-cleanup-D7-1759090-2.patch1.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch themesettings-CSS-cleanup-D7-1759090-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch for D7

Status:Needs review» Needs work

The last submitted patch, themesettings-CSS-cleanup-D7-1759090-2.patch, failed testing.

andrewmacpherson’s picture

andrewmacpherson commented
Issue tags:-needs backport to D7
StatusFileSize
new themesettings-CSS-cleanup-D8-1759090-4.patch1.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch themesettings-CSS-cleanup-D8-1759090-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Sorry, I think I was a bit too eager to mark this for D7 backport. After reading the backport policy, it seems that markup changes can only be accepted if it's a critical issue.

Uploading the D8 patch from #1 again, so the testbot knows which one to test.

andrewmacpherson’s picture

andrewmacpherson commented
Status:Needs work» Needs review
klonos’s picture

klonos commented

As for the D7 backport and only critical issues accepted: does that apply to even such minor changes? I mean it is code that is not used after all.

Mukeysh’s picture

Mukeysh commented

2: themesettings-CSS-cleanup-D7-1759090-2.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 2: themesettings-CSS-cleanup-D7-1759090-2.patch, failed testing.

Manjit.Singh’s picture

Manjit.Singh commented
Status:Needs work» Needs review

4: themesettings-CSS-cleanup-D8-1759090-4.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 4: themesettings-CSS-cleanup-D8-1759090-4.patch, failed testing.

InternetDevels’s picture

InternetDevels commented
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new drupal-remove_redundant_css_from_theme_settings_form-1759090-11.patch1.24 KB
PASSED: [[SimpleTest]]: [MySQL] 63,143 pass(es).
[ View ]
jhedstrom’s picture

jhedstrom commented
Status:Needs review» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Manjit.Singh’s picture

Manjit.Singh commented

Where to find theme settings file ?

jhedstrom’s picture

jhedstrom commented

The theme settings form moved to core/modules/system/src/Form/ThemeSettingsForm.php.

jjcarrion’s picture

jjcarrion commented
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new drupal-remove_redundant_css_from_theme_settings_form-1759090-15.patch1.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,820 pass(es).
[ View ]

I have just made the reroll

jhedstrom’s picture

jhedstrom commented
Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

This removes old CSS and an unused class, which is allowed as cleanup under the beta phase.

alexpott’s picture

alexpott commented
Status:Reviewed & tested by the community» Fixed

Committed d61f619 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed d61f619 on 8.0.x 
    Issue #1759090 by andrewmacpherson, InternetDevels, jjcarrion: Remove...

Status:Fixed» Closed (fixed)

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