| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | Minnelli theme |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D7 UX freeze |
Issue Summary
Right now, Garland and Minnelli are listed as two separate themes in the Theme administration interface. Now that we've removed many of the less-popular core themes and have added Stark and Seven, Garland/Minnelli are dominating this list.
I'd love to see these two become one theme. The only difference between them is in the width of columns: fluid-width vs fixed-width. By listing what is basically one theme twice, the Drupal interface is hinting "Pick this one! It's really, really good. It's so good, we are giving you two chance to choose it."
Looking at the code, Minnelli is a subtheme of Garland. It has a different images with the name "Minnelli" baked into them instead of "Garland" (the screenshot, the preview and base images in the color picker) but those images are otherwise the same: same size, same filetype, etc. Minnelli has it's own color picker, presumably because the color picker tool cannot support subthemes. And it has it's own stylesheet that overwrites three attributes of the Garland CSS, changing the widths of the wrapper container and sidebars to specific, fixed widths.
This could be one theme instead — named Garland — with one color picker, the Garland color picker sprites, a new screenshot (that doesn't say "fluid width") and a toggle switch in the theme settings that flips on the layout code from Minnelli that changes the theme to fixed width.
In general, Minnelli is just 11 lines of CSS. The newest Drupal best practices are to not make separate themes in a case like this, but to make one theme with theme settings. Since this is the practice we are encouraging for new core themes, it makes sense to start with Garland.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| 20091113-f1gqn7yh4an4e6j4ajymgi3819.jpg | 76.12 KB | Ignored: Check issue status. | None | None |
Comments
#1
Makes loads and loads of sense, on first glance at the screenshot, it looks like there's an error and Garland has been duplicated in the listing, would be great not to have that any more.
#2
My main concern with this patch once upon a time would've been that we don't have any other sub-themes in core and so it provides us with a way of testing them. But now we have an automated testing framework and the ability to add 'hidden' themes to do all sorts of crazy madness, so I would support this change.
Tagging as something to look at before UX freeze.
#3
Great idea, just keep in mind the upgrade path, i.e. resetting the theme if someone upgrades, and then migrating the blocks.
#4
Here's a suggestion for the text describing the merged theme:
A classic theme, well-loved by some in the Drupal community. This theme offers a tool for changing the background, text and link colors; and the ability to switch between fixed-width and fluid-width layouts.
Where "a tool" links to the settings page at: admin/appearance/settings/garland
#5
Yep, Jen, +1 for merging these. As a newbie, it baffled me why there were two nearly-identical themes.
#6
Here's updated screenshot.png, base.png, and preview.png that removes the "fluid width" text from those images.
The patch removes Minnelli completely.
The default width in the CSS is the fixed width option. If
garland_preprocess_html()adds afluid-widthclass, then the width is setup as fluid.garland_preprocess_html()will add thefluid-widthclass by default and/or if Garland is configured to use the new "Content width: fluid width" option on the new configure theme settings form for Garland (click "configure" next to Garland on the Appearance page.) The default width in the theme setting is the fluid width, so Garland will continue to be fluid width by default.Note that for the install, update, and db-offline maintenance pages the
garland_preprocess_html()function will not be run and the content width will use the CSS-defined default of fixed width. This is the equivalent of using Minnelli for those pages.#7
The last submitted patch failed testing.
#8
Wait, wait! Before you go down this path, let's get #547068: D7UX: use Seven for installation / updates in. It looks like you're changing almost exactly the same lines.
#9
I don't mind waiting for #547068: D7UX: use Seven for installation / updates to go in first. I have bazaar; I can re-roll anything. :-)
#10
Ok, done. GO. ;)
#11
Wait. We are removing the only sub-theme example here.
I'm totally opposed to this. This core implementation served very well as sub-theme example, and, instead of removing it, we should advance on it. Not only to have an example, but also to have a working implementation for testing.
#12
sun: We have a themes/tests/ directory where we have inheritance examples for testing. Minelli is a poor example of sub-theming anyway. There's no reason fixed vs. fluid couldn't be a setting, and then we could show off how to use the theme settings API.
I would absolutely love to have 10 gorgeous themes that piggyback off of Drupal's default template files to include in core as sub-theming examples, but not having those, this is a pretty poor case for leaving such a bizarre visual discrepancy.
#13
Ditto what webchick said. :-)
Here's a re-roll hopefully fixing the MenuIncTest.
#14
The last submitted patch failed testing.
#15
JohnAlbin requested that failed test be re-tested.
#16
+1. Patch works well and is easy to understand for a semi-newb like me, which is always a plus.
#17
No upgrade path?
#18
Added upgrade path for catch. :-)
#19
Looks great. Didn't test any of it but from a code point of view it's a lot cleaner and the upgrade path looks sane.
#20
Good idea.
#21
Per catch's sharp eyes in IRC, this patch fixes the coding style on an inline comment in garland/theme-settings.php. No code changes, so leaving at RTBC.
#22
+++ modules/simpletest/tests/menu.test 2009-11-22 21:59:17 +0000
@@ -56,11 +56,12 @@ class MenuIncTestCase extends DrupalWebT
function testThemeCallbackMaintenanceMode() {
variable_set('maintenance_mode', TRUE);
+ variable_set('maintenance_theme', 'seven');
// For a regular user, the fact that the site is in maintenance mode means
// we expect the theme callback system to be bypassed entirely.
$this->drupalGet('menu-test/theme-callback/use-admin-theme');
- $this->assertRaw('minnelli/minnelli.css', t("The maintenance theme's CSS appears on the page."));
+ $this->assertRaw('seven/style.css', t("The maintenance theme's CSS appears on the page."));
This is breaking the test's purpose.
See my patch in the other maintenance theme issue on how to alter it properly.
I'm on crack. Are you, too?
#23
+ variable_set('maintenance_theme', 'seven');
// For a regular user, the fact that the site is in maintenance mode means
// we expect the theme callback system to be bypassed entirely.
$this->drupalGet('menu-test/theme-callback/use-admin-theme');
- $this->assertRaw('minnelli/minnelli.css', t("The maintenance theme's CSS appears on the page."));
+ $this->assertRaw('seven/style.css', t("The maintenance theme's CSS appears on the page."));
What @sun said :)
More specifically, the test only serves a purpose if it visits a page whose default theme is different from the maintenance theme. You should just be able to get rid of the variable_set() that was added and then assert 'garland/style.css'....
+ if (variable_get('theme_default', 'garland') == 'minnelli') {(minor) We really don't need 'garland' as the variable_get() default here - just
variable_get('theme_default') == 'minnelli'should be fine.+ // Set the theme setting width to "fixed" to match Minnelli's old layout.+ $settings = variable_get('theme_garland_settings', array());
+ $settings['garland_width'] = 'fixed';
+ variable_set('theme_garland_settings', $settings);
Can't Minelli have 'theme_minnelli_settings', color module settings, etc that should be migrated over as well?
+ if (variable_get('admin_theme', 'seven') == 'minnelli') {+ variable_set('admin_theme', 'seven');
+ db_update('system')
+ ->fields(array('status' => 1))
+ ->condition('type', 'theme')
+ ->condition('name', 'seven')
+ ->execute();
+ }
Why would we switch them to Seven here? If they're using Minnelli as their admin theme, shouldn't we replace that with Garland?
+ '#weight' => '-2',(minor) Does this really need to be a string?
#24
Updated patch given sun's and David's comments.
The color files are generated using color.module's
color_scheme_form_submit()(a submit handler forform_system_theme_settings_alter). :-p I don't see an easy way to re-create this functionality inside an system update function, so I've just transfered the Minnelli's form settings to Garland; users will just have to go to that form and hit submit to re-create all the color files.#25
Bot
#26
No code review from me, but definately a UX +1 to doing this for all the obvious reasons already mentioned here.
#27
Actually applied and tested this patch. Works as advertised. It's a great relief to not see the same preview image twice on the Appearance page anymore.
Since it has been RTBC before, setting it back to that. Don't forget to commit the images in #6 as well.
#28
The patch looks good. It did occur to me, though, that the update path probably still isn't complete - thinking about things like themes which are enabled but not the default, $user->theme, etc, which are not dealt with in this patch... deleting a theme gets complicated very fast :)
However, it's probably good enough for starters, so we could try to perfect the update path later - or, maybe we could decide to just move Minnelli to contrib (where it could then evolve into something better) rather than killing it completely, and then we don't need to worry about an update function at all?
#29
Didn't actually mean to change the status - like I said above, this seems good enough to commit.
#30
Oh, I'd like to point out that now that #491214: implement the top level Appearance / Choose Theme admin page has been committed, we don't need the screenshot.png in comment #6 anymore. Also, the patch in #24 needs a re-roll since it doesn't apply anymore due to changes in default.settings.php.
So here's a re-rolled patch. And the 2 updated color files for Garland again.
#31
The last submitted patch failed testing.
#32
Ah, looks like the testAdministrationTheme test produces a warning when there are no disabled themes. Fixed that. No other changes. Images from comment #30 are still the ones to use.
#33
The last submitted patch failed testing.
#34
Whoops. Didn't quite fix that last test failure. Now I have. (I hope.) :-)
Again, images from comment #30 are still the ones to use.
#35
Yay! Committed to HEAD. I think. ;) That was pure CVS hell; it'd be nice for someone to confirm everything got there okay.
#36
Just did a cvs up. Looks good to me! :-)
#37
Hmm. I happened to have minnelli as the default theme on my sandbox, so I did cvs up, and now update.php is WSOD...
#38
@matt2000. But update.php uses the Seven theme to display its content, so the removal of Minnelli wouldn't affect its ability to be displayed. I don't think this issue has anything to do with your WSOD on update.php.
#39
Automatically closed -- issue fixed for 2 weeks with no activity.