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.
Comment | File | Size | Author |
---|---|---|---|
#34 | merge-garland-632030-34.patch | 13.27 KB | JohnAlbin |
#32 | merge-garland-632030-32.patch | 13.19 KB | JohnAlbin |
#30 | garland-color-images-632030-30.tgz | 38.55 KB | JohnAlbin |
#30 | merge-garland-632030-30.patch | 12.48 KB | JohnAlbin |
#24 | merge-garland-632030-24.patch | 12.71 KB | JohnAlbin |
Comments
Comment #1
catchMakes 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.
Comment #2
webchickMy 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.
Comment #3
mikey_p CreditAttribution: mikey_p commentedGreat idea, just keep in mind the upgrade path, i.e. resetting the theme if someone upgrades, and then migrating the blocks.
Comment #4
jensimmons CreditAttribution: jensimmons commentedHere'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
Comment #5
lisarex CreditAttribution: lisarex commentedYep, Jen, +1 for merging these. As a newbie, it baffled me why there were two nearly-identical themes.
Comment #6
JohnAlbinHere'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-width
class, then the width is setup as fluid.garland_preprocess_html()
will add thefluid-width
class 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.Comment #8
webchickWait, 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.
Comment #9
JohnAlbinI don't mind waiting for #547068: D7UX: use Seven for installation / updates to go in first. I have bazaar; I can re-roll anything. :-)
Comment #10
webchickOk, done. GO. ;)
Comment #11
sunWait. 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.
Comment #12
webchicksun: 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.
Comment #13
JohnAlbinDitto what webchick said. :-)
Here's a re-roll hopefully fixing the MenuIncTest.
Comment #16
mcrittenden CreditAttribution: mcrittenden commented+1. Patch works well and is easy to understand for a semi-newb like me, which is always a plus.
Comment #17
catchNo upgrade path?
Comment #18
JohnAlbinAdded upgrade path for catch. :-)
Comment #19
catchLooks 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.
Comment #20
chx CreditAttribution: chx commentedGood idea.
Comment #21
JohnAlbinPer 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.
Comment #22
sunThis 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?
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedWhat @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'....
(minor) We really don't need 'garland' as the variable_get() default here - just
variable_get('theme_default') == 'minnelli'
should be fine.Can't Minelli have 'theme_minnelli_settings', color module settings, etc that should be migrated over as well?
Why would we switch them to Seven here? If they're using Minnelli as their admin theme, shouldn't we replace that with Garland?
(minor) Does this really need to be a string?
Comment #24
JohnAlbinUpdated 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.Comment #25
mcrittenden CreditAttribution: mcrittenden commentedBot
Comment #26
yoroy CreditAttribution: yoroy commentedNo code review from me, but definately a UX +1 to doing this for all the obvious reasons already mentioned here.
Comment #27
yoroy CreditAttribution: yoroy commentedActually 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.
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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?
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedDidn't actually mean to change the status - like I said above, this seems good enough to commit.
Comment #30
JohnAlbinOh, 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.
Comment #32
JohnAlbinAh, 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.
Comment #34
JohnAlbinWhoops. Didn't quite fix that last test failure. Now I have. (I hope.) :-)
Again, images from comment #30 are still the ones to use.
Comment #35
webchickYay! Committed to HEAD. I think. ;) That was pure CVS hell; it'd be nice for someone to confirm everything got there okay.
Comment #36
JohnAlbinJust did a cvs up. Looks good to me! :-)
Comment #37
matt2000 CreditAttribution: matt2000 commentedHmm. I happened to have minnelli as the default theme on my sandbox, so I did cvs up, and now update.php is WSOD...
Comment #38
JohnAlbin@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.