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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Priority: Normal » Critical
Issue tags: +webchick's D7 alpha hit list

This 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.

mrfelton’s picture

Status: Active » Needs review
FileSize
59.82 KB
64.61 KB
55.37 KB
3.87 KB

I 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:

div.add-or-remove-shortcuts {
  float:left;
  padding-left:6px;
  padding-top:6px;
}

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:

h1.with-tabs {
  float:left;
  margin:0 2em 0 0;
  padding:0;
}

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.

cweagans’s picture

FileSize
753 bytes

I 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)

aspilicious’s picture

I like mrfeltons patch...

mrfelton’s picture

FileSize
38.07 KB

@aspilicious: That just looks messy to me. At least move it away from the tabs a little so that it doesn't overlap.

cweagans’s picture

I'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.

mrfelton’s picture

Sorry, 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?...

cweagans’s picture

Sure 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)).

mrfelton’s picture

@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.

webchick’s picture

Status: Needs review » Needs work
Issue tags: +Usability, +Needs design review

Yeah, 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...

aspilicious’s picture

Maybe 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.

webchick’s picture

I 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. ;)

Bojhan’s picture

Is 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?

David_Rothstein’s picture

Right, 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.

Bojhan’s picture

I agree with @David here, this is not how it was intended.

webchick’s picture

Title: The "add to shortcuts" link appears in Garland/Stark and looks ugly » Move add/remove shortcut buttons to a theme setting

Ok, adjusting title.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
3.45 KB

Here'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.

David_Rothstein’s picture

Huh, 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.

webchick’s picture

Status: Needs review » Needs work

Hm. 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

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

Now with a better name for the theme setting.

David_Rothstein’s picture

Oh... 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?

bleen’s picture

Hm. 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..

Couldn'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?

David_Rothstein’s picture

OK, 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.

webchick’s picture

Status: Needs review » Fixed

Yeah, 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.

aspilicious’s picture

Status: Fixed » Needs work

Did someone tested this? Cause i think this caused the issue that there aren't any shortcut buttons anymore (read: no shortcuts in seven admin)

aspilicious’s picture

Status: Needs work » Fixed

Reinstall fixed it. Aparantly the .info file doesn't get updated automaticly...

David_Rothstein’s picture

I added theme upgrade documentation at http://drupal.org/update/theme/6/7#shortcuts

Status: Fixed » Closed (fixed)

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

Jacine’s picture

Hey, 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?

jensimmons’s picture

I 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?

yoroy’s picture

Status: Closed (fixed) » Active

So, opening up the issue again

David_Rothstein’s picture

Title: Move add/remove shortcut buttons to a theme setting » Move add/remove shortcut buttons to a theme setting and make it work for different themes
Status: Active » Needs work

@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:

settings[shortcut_module_link] = 1

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:

  • The plus link needs to look good by default in themes besides Seven (and in particular would be nice if it can work with Garland). I'm moving this to "needs work" since there are some patches earlier in the issue that started out on this. As per #14, I intended to reopen this issue for that after the initial commit was made, but apparently forgot. Oops :)
  • Maybe we need to expose this theme setting in the UI after all, and somehow give site administrators the ability to only turn it on for the admin area of their site? It was primarily designed for admin pages I guess, but as long as the theme supports it and the site administrator wants it, I don't see any reason to prevent it from being available on the "front end" of the site also. Certainly there are pages there that might be useful for people to bookmark.
jensimmons’s picture

Re:#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.

Jacine’s picture

I 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.

jensimmons’s picture

Yeah, I agree with Jacine, that it would make more sense as a module setting. Some kind of setting. Not on by default.

David_Rothstein’s picture

Hm, 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.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
4.97 KB

OK, 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.

sun’s picture

Status: Needs review » Needs work

I 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.

David_Rothstein’s picture

Regarding 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:

  1. Are there good reasons to keep this a theme setting? I think the answer to that depends on whether its presence on the page has the ability to ruin a theme. Certainly it can ruin it at the moment - the module's CSS is all tied up with Seven, so to make it look reasonable at all, each theme has to style it itself. If we fix that, things might be different.
  2. Does core need to provide a UI at all? It's certainly simpler if we don't. Note that the condition in #37 could be made somewhat more sensible just by changing it from 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.
catch’s picture

Priority: Critical » Normal

Not a critical bug.

YesCT’s picture

is this still on the alpha hit list?

David_Rothstein’s picture

Title: Move add/remove shortcut buttons to a theme setting and make it work for different themes » The add/remove shortcut buttons look bad and don't appear in most themes besides Seven
Priority: Normal » Major

We 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.

David_Rothstein’s picture

After 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:

  • Makes it so the shortcut links are now properly limited to admin pages, unlike before ("Seven" != "admin pages"). This is now possible due to the recent commit at #669510: Merge administration theme with hook_admin_paths()). Contributed modules can still override this, of course.
  • Consequently, Bartik is now able to display the shortcut link whenever it is being used on an administrative page (e.g. if you have Bartik as your admin theme or if you aren't using an admin theme and instead see Bartik everywhere).
  • Minor fixes to the Bartik shortcut styling (it looks like it had gotten a bit out of date, not surprising since it was never being used).

I'm also attaching a couple screenshots, showing Bartik on an admin page both inside and outside the overlay, with this link visible.

bleen’s picture

I 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...

David_Rothstein’s picture

Yes, 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.

sun’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » task
Priority: Major » Normal
Issue tags: -webchick's D7 alpha hit list

I 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.

Bojhan’s picture

Fyi, I am pretty sure its unlikely we are going to keep this pattern. Implementation a side.

webchick’s picture

Indeed. 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..

Bojhan’s picture

Status: Needs review » Closed (won't fix)

We 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.