Move theme shortcut CSS to the Seven theme

Out of the box shortcuts module supports all the core themes so moving to classy makes sense but I'd be reluctant to move it to seven.

Moving to classy means that for every conrtib them which supports shortcuts has to be a subtheme of classy. Neither omega nor bootstrap is using classy as a base theme and @JohnAlbin hasn't made any decision to use classy as a subtheme for zen but I have an inside news that he is not planing to use classy for zen. Shiny doesn't have a D8 branch yet so I don't know about it. adaptivetheme does use classy as base theme. Given all that moving to classy doesn't make sense as well.

I just had a quick look at shortcut.theme.css & shortcut.icons.theme.css all the classes are added in shortchut.module file. IMO the css file names are wrong because this CSS is not specific to any theme at all.

If we end up doing this anyway I would like to see all the (ltr+rtl) screenshots from all the themes in core just to make sure that we are not introducing any visual regression here.

I'm not a front-end expert so I might be missing a bigger picture here but I have done my due diligence as a module maintainer.

Thanks for creating the thorough IS.

Do you consider shortcuts to be an admin widget?

I think shortcuts functionality is more helpful for content editor. It helps admin to create workflow for content editor with useful links for them.
+1 to move it to classy.

theme maintainers have a choice if they want to style it themselves or they want to use the default styling.

+1 to this as well.

Need to be reviewed.

Re-rolled with -C -M, looking at that one now. Not sending for testing, because it's the same code.

Re-rolled with -C -M, looking at that one now. Not sending for testing, because it's the same code.

Please post before and after screen shots of all themes so that we can confirm there is no regression after this. To enable shortcuts for themes other then seven see #2544390: Fix shortcut action link styling in bartik.

I want to provide another solution for this issue. This patch provides moving all CSS to Seven, declaring library 'drupal.shortcut' in Seven theme, including this library in Classy theme. As Bartik theme is based on Classy, the shortcut style works there. If custom theme don't want to use a Classy base theme, but wants to use a default shortcut style, then library could be included like in Classy.
I would like avoid using this alter hook in the core modules, doesn't seem clean for me.

+++ b/core/themes/classy/classy.info.yml
@@ -8,3 +8,4 @@ core: 8.x
+  - seven/drupal.shortcut

I'm all for avoiding alter hook but this doesn't seem right.

Other then this I think this is ready and an interdiff please. :)

@jibran I agree with you, then we should move CSS to Classy instead Seven.

I +1 that in #5. We need classy maintainer to sign this off.

Gints, do you have an updated local install? I'm pretty sure we killed the shortcut star functionality when we converted page title to a block.

Something doesn't seem right here. That last patch is adding the library in Classy's info file which includes it globally, which we definitely don't want. This would be an administrative function, only available to people with edit shortcut ability. It is removing the library from shortcut module, but not modifying it's conditional inclusion. You'd have to us the alter to keep it working the same way.

However, I'm still not seeing the benefit of moving this to Classy. It's admin functionality with styling provided by the admin theme. Admin themes in all likelihood would want to style it differently, and provide their own icons, which is why I think we are considering its current styling a property of Seven.

davidhernandez,

Gints, do you have an updated local install? I'm pretty sure we killed the shortcut star functionality when we converted page title to a block.

Yes, I have updated local install. The shortcut for frontend theme was enabled for testing Bartik. And there the idea comes from about including shortcut CSS globally for all Classy based themes.
Do you agree with declaring shortcut css as library in Seven theme, so other themes can include that library as needed?

Shortcut library included only in Seven theme.