Previously the "add to shortcuts" link only was shown in the Seven theme (but via a hackish method).
As a side effect of #646874: Modules like Contextual links and Shortcut cannot add to "template regions", it now appears in all themes, but as you may have noticed, it looks really bad in Garland (particularly the way it interacts with local tasks). See screenshot for one example, and note the random placement of the "+" icon, which expands into the "add to shortcuts" link.
I think the simplest thing to do here (as well as the one that is pseudo-API-changing and so needs to be done soon) is turn the "add to shortcuts" link into a proper theme setting/feature. Then, Seven can choose to support this feature, while other themes do not have to.
Ideally, we would also make the link look good in Garland and Stark and then consider whether to display it there or not; the fact that it looks so bad suggests that there is perhaps some Seven-specific code in the CSS provided by the shortcut module.
Comments
Comment #1
webchickThis is a really obvious bug that immediately slaps people in the face with ugly, so I'd love to get it sorted out prior to alpha release.
Comment #2
mrfelton CreditAttribution: mrfelton commentedI don't think there is any seven specific code in the shortcut.css, but seven does make extra effort to style those shortcut links using:
But I think the real problem is in the fact that garlands tabs are displayed directly after the page heading, and the page heading has a load of right-margin to add spacing between it and the tabs:
This will never work as the shortcut link expands when hovered over, and because the tabs are so clost to the shortcut link it ends up shoving the tabs over to display the full hover text.
If that margin was removed, and the tabs were floated to the right like they are on seven the problem would go away. However, that means floating garlands tabs to the right, which obviously changes the look of garland a fair amount.
Here is a patch that fixes the issue by floating the tabs to the right, adding an extra 'page-title' class to the h1, and adding an extra wrapper div around the title and tabs with clearfix (works in much the same way as seven). Also a few screenshots to show the various states.
Comment #3
cweagansI do not think that a visual style change for Garland is necessary (or even an option at this point). Why not simply stick it right under the title? Patch attached.
EDIT: this patch doesn't work on the front page, or any admin/* page where there's a bunch of other admin links listed (like admin/structure)
Comment #4
aspilicious CreditAttribution: aspilicious commentedI like mrfeltons patch...
Comment #5
mrfelton CreditAttribution: mrfelton commented@aspilicious: That just looks messy to me. At least move it away from the tabs a little so that it doesn't overlap.
Comment #6
cweagansI'm assuming that was @ me. I didn't have a whole lot of time to spend on it, so it's really a quick rather crappy patch, but you get the idea, no? My point is that there's easier ways to make the shortcut link not look like crap without majorly changing Garland.
Comment #7
mrfelton CreditAttribution: mrfelton commentedSorry, yes, cweagans that was intended at you :)
I don't really see that easy or hard is an issue here, neither approach is hard it's just a question of which one looks best. If altering garland by moving the tabs from the left to the right is a change that is too major, then your approach seems fine to me. Otherwise, I think my one works better. Lets get some other opinions in it please?...
Comment #8
cweagansSure thing. I just asked for opinions in IRC and it seems that nobody came here...probably when the US wakes up again, there will be more people.
Anyways, the problem is that Garland has been around since Drupal 5, and it's looked and functioned more or less the same. To move the tabs from one side of the screen to the other doesn't seem like a big change, but for people who have been using Garland since 5.x, it would be frowned on, I think. Also, any documentation that has screenshots of any part of the interface using the Garland theme would be invalidated (which is bad -- we aren't invalidating any documentation since string freeze (Nov 15, 2009, per http://drupal.org/node/578446)).
Comment #9
mrfelton CreditAttribution: mrfelton commented@cweagans: It's just that the + feels a little out on a limb and unconnected to the title when it is displayed underneath it. I think the functionality of the link is conveyed a lot better when it is placed beside the title, though I totally understand what you are saying about breaking documentation/confusing people by changing garland. My question is though (although unrelated to this thread) why the hell is garland still being used as the primary Drupal theme?! No disrespect to whoever made it, but it makes Drupal look bad.
Comment #10
webchickYeah, sorry mrfelton, but I agree with cweagans that I don't think we can really make that kind of a major UI change to Garland at this point in the release cycle. Also, in tests, people have a hard enough time seeing those tabs right now when they're right next to the heading. It's possible they'd see them better if they were moved even further away, but I think exploring that is D8 material at this point...
I'll tag for the UX and design teams though, because if we go with cweagan's fix, I agree that we need to tweak the output.
And we still have Garland because no one delivered in #293540: New theme to include for core. :( Maybe next time...
Comment #11
aspilicious CreditAttribution: aspilicious commentedMaybe you should make mockups for a new drupal theme on next drupalcon? Maybe there are soom themers interested in making this, but don't have any idea how it should look like... But that is D8 material.
Comment #12
webchickI am not in any way a designer, so there's no way I'm making mocks. ;)
But yes, let's not take this issue off-target, please. I'd really love this to get fixed before Friday, and if we start ranting/lamenting Garland as the default core theme in D7, we will still be doing so in December. ;)
Comment #13
Bojhan CreditAttribution: Bojhan commentedIs there any reason we have this in Garland if its not your admin theme - shortcuts where never itended for pages outside of /admin? Anyway, the style is most definitively not representative to how it should look in Garland. This indeed has to has to get a specific implementation per theme.
I imagine below could work, but its not ideal at all - since it takes up a whole line of space? I don't know, perhaps just hiding it on Garland isn't too bad?
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedRight, as I explained above (in the original bug report), it was an unintended side effect that this started to appear in Garland. To fix this properly, we need to make it a theme setting, which allows themes to choose if they want to support it.
Later, we can try to make sure the existing CSS is in the proper place (Seven vs Shortcut module) and perhaps implement it in Garland if we want to, but those should be done afterwards...
I'll see if I can work on this today, but I have other patches I'm working on too, so no guarantees.
Comment #15
Bojhan CreditAttribution: Bojhan commentedI agree with @David here, this is not how it was intended.
Comment #16
webchickOk, adjusting title.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a quick lunchtime patch :)
It seems to mostly work, although the code seems a little odd to me - is there (or should there be) a better way for a theme to enable an optional setting than resorting to what I did here?
Also, it seems to introduce a (small) bug, whereby disabling and reenabling the shortcut module in the overlay causes the styling of the shortcut bar to be messed up initially. We can probably save that one for a followup.
Note by the way that there is a similar issue at #629160: Allow contextual links to be disabled for particular themes (though it needs a reroll) - might be a good one to review/commit while looking at this one. I don't think that one affects any core themes, but could easily cause a similar kind of problem for certain themes in contrib.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedHuh, well I actually just realized there is a simpler way.
Drupal does not require that every theme setting has a UI, and there probably isn't a need for this particular one to have it. Declaring it in the .info file is enough.
So this patch is probably better than the last one. Any theme that wants to display the add/remove shortcut link simply has to add a single line to their .info file, and it should automatically appear.
Comment #19
webchickHm. Not sure. Contrib modules can't cheat like that, and tell all themes to implement a special info file property just for them. I'm not real inclined to have core setting this example, especially when Shortcut is an optional module...
But if that's the only way to do it, could we please use a shorter name, like "shortcut_link"? While undoubtedly more clear, I don't think I could ever type a string like "+settings[toggle_add_or_remove_shortcut] = 1" without making a typo somewhere in there. :P
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedNow with a better name for the theme setting.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedOh... crosspost. Well, my independent idea of a better name was sort of along the lines of yours, but not quite :) I think 'shortcut_link' might sound too much like 'shortcut_icon' though (which as I recall is also a theme setting).
I don't know how to make an optional theme setting that doesn't require the themes to be aware of it... maybe someone else does?
As far as I know, other optional core modules (like comment module) do some pretty bad things here as well (comment module theme settings code is all over system module). In that case the theme settings is opt-out rather than opt-in like this one, but I think that's the only difference?
Comment #22
bleen CreditAttribution: bleen commentedCouldn't we use form_alter (within the shortcut module) to add a checkbox to /admin/appearance/settings/%theme and turn the setting on (by default) for seven and off for all others themes?
This idea isn't 100% fleshed out, but I think it can be made to work... thoughts?
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedOK, I rerolled with 'shortcut_module_link' for the theme setting name. Hopefully good?
@bleen18, I think your idea would work, but wouldn't the "turn the setting on (by default) for seven and off for all others themes" part still require Seven to be "aware" of the Shortcut module? Also, I think we ideally should leave this out of the UI, since it's probably giving users more knobs to turn than they really need.
Comment #24
webchickYeah, on second look, I like this. It solves the problem in a generic way, using existing tools that are used to solve similar problems from other core modules. And big kudos for the name change. :D
Committed to HEAD.
This'll need to be documented in the theme upgrade docs. Admin themes will want to toggle this setting.
Comment #25
aspilicious CreditAttribution: aspilicious commentedDid someone tested this? Cause i think this caused the issue that there aren't any shortcut buttons anymore (read: no shortcuts in seven admin)
Comment #26
aspilicious CreditAttribution: aspilicious commentedReinstall fixed it. Aparantly the .info file doesn't get updated automaticly...
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedI added theme upgrade documentation at http://drupal.org/update/theme/6/7#shortcuts
Comment #29
JacineHey, I'm having a WTF moment about this setting. What I'm seeing here is:
1. The shortcut module was never intended for use outside of the admin area.
2. If you use Garland you don't get to use this functionality because it would mean UI changes, so basically Garland is no longer a fully functional option for an admin theme in Drupal 7.
It seems weird to me that this shortcut module is only for the administrative area. Is this really the case?
If so, then what happens when a theme is made to be used in both the back end and front end of a site? Themers are supposed to page_alter (or something) to get rid of the markup or display: none on non-admin pages?
Comment #30
jensimmons CreditAttribution: jensimmons commentedI don't fully understand everything that's going on, but what I do know is that Bartik has a big plus link all over front-facing content pages. And that's sad. Not just for Bartik, but for every theme that will ever be built in D7.
Can we get this fixed?
Comment #31
yoroy CreditAttribution: yoroy commentedSo, opening up the issue again
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commented@jensimmons, this feature is opt-in (see http://drupal.org/update/theme/6/7#shortcuts). I took a look at the latest CVS code for Bartik and at the bottom of the bartik.info file is this line:
Remove that line and it won't be there anymore.
***
That said, I think some of @Jacine's points in #29 are correct and there is more to do here. Two things perhaps:
Comment #33
jensimmons CreditAttribution: jensimmons commentedRe:#32
Right. The plus shows up because we added
settings[shortcut_module_link] = 1
to bartik.info.
We did that so that Bartik could be used as an admin theme. Of course, it's expected that the majority of people will use Seven as the admin theme, but we wanted to make sure Bartik doesn't break, doesn't look terrible and does function as an admin theme. Including the shortcut buttons is part of this.
The desired behavior, however, is for it only show for URLs at /admin, and not for everything. If people can imagine a situation where they would want shortcuts on every page, then make it a theme setting, or a code setting in the .info file.
Comment #34
JacineI agree with both of @David_Rothsteins comments in #32.
I don't think this should be a theme setting. A module setting would give the user easy control over what they want to do with it without having to go into the theme layer.
Comment #35
jensimmons CreditAttribution: jensimmons commentedYeah, I agree with Jacine, that it would make more sense as a module setting. Some kind of setting. Not on by default.
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedHm, I think the rationale for keeping it a theme setting is that we need a clean way to e.g. turn it on for the admin theme and off otherwise. But perhaps it should be an "opt-out" setting - like the other core theme settings - rather than an "opt-in" setting like it is now? (At least once it is actually made to look nice in some other theme besides Seven...)
Also, the shortcut module could maybe play around with trying to turn it on or off automatically as needed when a new theme is enabled, so that most users won't have to fiddle with it much.
I think there is definitely a use case for allowing it to be shown on non-admin pages, especially for a site that is not using an admin theme. For them, the distinction of pages under /admin might not even mean much.
Comment #37
David_Rothstein CreditAttribution: David_Rothstein commentedOK, thinking about it a bit more, I guess we could do something like this patch... Get rid of the theme setting (as suggested), put it only on admin pages by default, but in a way that is easy for other modules to override. (Note that most of this patch is just indentation changes.)
Or possibly add a setting in core to override it, since if not using an admin theme also, I'm still not sure it really makes sense to limit this only to admin pages. But this patch (plus CSS/markup fixes that are guaranteed to make it not break other themes) might be better than the current situation.
Comment #38
sunI agree that the committed patch was wrong. I don't agree with the last conclusion/quickfix. Why should Shortcut module suddenly be limited to administration pages? I can perfectly build a view for content moderators (read: moderators, not administrators) on my site, and those moderators likely want to put that page in their shortcuts.
@David: Since you already analyzed possibilities to allow modules expose theme settings, could you summarize what's holding us off from implementing it that way? It's the only valid solution for this issue, IMO.
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedRegarding module-exposed theme settings, see #629826: Theme setting toggles for comments appear when the comment module is disabled for how comment module "does" it (or grep for 'comment_user_picture'). It's not pretty, and done entirely in system module, not even by comment module itself - basically it's broken :) Instead, we might be able to do something like what @bleen18 suggested in #22, at least at the UI level. Things will probably get messy when dealing with saving it to the database or defining intelligent default values, though, just because of the way theme settings in Drupal work (all stored together in the same array, with complex rules for overriding).
However, it sounds from above like there are questions about whether this belongs in that area of the UI at all? The administrator will probably not expect to have to go to the theme settings page to turn it on again, e.g. after they have switched themes. Hence the idea of making it a separate setting (outside of the theme system) or giving up and just trying to define a sensible default - with the idea that contrib modules could override it (or provide a UI) for other use cases.
Thus, there are two questions:
path_is_admin(current_path())
to something more like!variable_get('admin_theme') || path_is_admin(current_path())
. The module will certainly be more powerful if it provides a UI for making this configurable, but leaving it to a hypothetical "Advanced shortcuts" module is not so terrible either.Comment #40
catchNot a critical bug.
Comment #41
YesCT CreditAttribution: YesCT commentedis this still on the alpha hit list?
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedWe really need to fix this. Without it, the shortcut module is pretty much crippled in themes besides Seven.
I guess it's getting late to add a setting for this in the UI, but the world won't end if we hardcode it to admin pages (since contrib modules should be able to override that behavior easily). The important thing is to fix the CSS.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedAfter trying all the CSS tricks I know, I've convinced myself there's no theme-independent way to do this in Drupal 7. (There are people in this issue who know a lot more theming tricks than I do, though, so I'm still holding out a little hope.)
This is something we should fix in Drupal 8 because the use case of a module wanting to append an icon to the page title without affecting the theme is not an unreasonable one. But I don't see any way to do that now without us changing the way the title is rendered, which is too late to do.
So, for Drupal 7 we'll have to live with not showing the shortcut link except when the theme has chosen to opt-in :(
However, all is not lost! I discovered in #845882: Bartik has unused shortcut-related code that Bartik already has code to display the shortcut link without breaking the theme, and it's silly that that code isn't being used. We shouldn't be turning off functionality in Bartik (or any other theme that supports it) just because it isn't a dedicated admin theme.
Here is a simple patch that fixes that:
I'm also attaching a couple screenshots, showing Bartik on an admin page both inside and outside the overlay, with this link visible.
Comment #44
bleen CreditAttribution: bleen commentedI have some vague recollections about this from like a year ago, and there was definitely some discussion about limiting the add/remove shortcut link to admin only pages, but those are not the only pages that a person would necessarily want to shortcut. Examples: /node or /node/123 (an organic group lets say)
Not sure how I feel about this...
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedYes, but basically unless you display your organic groups in the Seven theme, you won't see a link on them now either. So, we have to work with what we've got, which is what we've had all along :(
And as mentioned, another module can always override this.
Comment #46
sunI still disagree with the direction of this patch (see my earlier comment). Looks like D7 themes will need to manually implement proper support for Shortcut module, and we can only resolve this properly for D8.
Comment #47
Bojhan CreditAttribution: Bojhan commentedFyi, I am pretty sure its unlikely we are going to keep this pattern. Implementation a side.
Comment #48
webchickIndeed. Only one of 8 participants was able to divine what the heck that "+" was for, and that was only by accidentally stumbling across it. See also #1164782: The icon to add something to shortcuts wasn't clearly discoverable..
Comment #49
Bojhan CreditAttribution: Bojhan commentedWe are going to continue the discussion in #1164782: The icon to add something to shortcuts wasn't clearly discoverable.. It is clear this direction is not the right one, when we require each theme to implement it correctly - and the patterns explored in the other issue will solve this problem much more gracefully.