This issue is split off from #682000: Remove the default limit of 7 shortcuts per shortcut set, but Drupal 8 only.

The proposal that was suggested was to completely remove all code related to limiting the number of shortcuts per shortcut set via the UI (whereas the patch in the other issue merely sets the default to have no limit, in a way that is intended for D7 backport, but still allows limits to be imposed by setting a variable).

There are some pros and cons to this; read up on the above issue for more details.

One thing that I know someone will bring up so I'll bring it up now myself; if we no longer enforce a limit, do we need to have the concept of enabled/disabled shortcuts anymore? (It's a related question, but perhaps it can it be deferred to a separate issue?)

CommentFileSizeAuthor
#12 1304486-12.patch14.02 KBswentel
#9 core-1304486-9.patch14.17 KBnod_
#3 core-1304486-3.patch13.85 KBnod_
#2 1304486-2.patch14.07 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

We shouldn't, the whole enabled/disabled shortcuts is freaking confusing anyway.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Active » Needs review
FileSize
1304486-2.patch14.07 KB

Yes, let's kill this!

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-1304486-3.patch13.85 KB

reroll. Works fine for me let's see what testbot says.

Makes me happy to see so much JS removed :)

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community

Let's do this! :)

Dries’s picture

Dries CreditAttribution: Dries commented

#3: core-1304486-3.patch queued for re-testing.

Dries’s picture

Dries CreditAttribution: Dries commented

I'm asking for a re-test because it did not seem to apply locally.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, core-1304486-3.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Probably needs a re-roll due to #1684866: JSHint shortcut.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review
FileSize
core-1304486-9.patch14.17 KB

reroll

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community

Applies cleanly now

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Thanks for the re-roll. Committed to 8.x. Thanks.

swentel’s picture

swentel CreditAttribution: swentel commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Needs review
FileSize
1304486-12.patch14.02 KB

Here's the D7 version

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Title: Completely remove the ability to limit the number of shortcuts per set » Completely remove the ability to limit the number of shortcuts per set (upgrade path)
Version: 7.x-dev » 8.x-dev
Priority: Normal » Major
Status: Needs review » Needs work

This needs some kind of an upgrade path - otherwise your disabled shortcuts will be listed in the UI but with no way to reenable them (except by deleting and readding). And it's confusing too, since they show up on the admin screens but there's no indication they're disabled.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

I also don't see how this patch can ever be backported to Drupal 7? It makes major UI changes, changes function signatures, removes other functions, etc...

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Title: Completely remove the ability to limit the number of shortcuts per set (upgrade path) » Completely remove the ability to limit the number of shortcuts per set (upgrade path and followup)

http://drupal.org/node/1358140#comment-6320784 identifies some out-of-date documentation that was introduced here also.

xmacinfo’s picture

xmacinfo
he/him
Primary language French
Location Canada
CreditAttribution: xmacinfo commented

A possible duplicate with a patch is here #682000: Remove the default limit of 7 shortcuts per shortcut set for Drupal 7.

Please note that we do not need to make any changes to add more than 7 shortcuts. By clicking on “display line weight” link on the shortcut page, you can add as many shortcuts as you like. No need to disable JavaScript or apply a patch to Drupal.

I am leaving version and priority as is for this issue, since the focus is to properly remove the limits in D8.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

I think better to convert a shortcut set to ConfigEntity and using EntityNG provide a ability to store entity references to new menu-items see #916388: Convert menu links into entities

About upgrade path: suppose we just need to implement a block setting to limit a number of links to display. No idea for now how to make this but having to make this setting per user or role #1122816: Assign shortcut set to role

EDIT filed #1811640: Convert shortcuts into configuration

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

I'm not sure about removing this setting. Seems it's even more important in the world of mobile stuff? When we did usability testing on the mobile toolbar patch, it was noted that "Almost all participants (5 of 6 participants) had trouble noticing the "Edit shortcut" and thought need a little more visual emphasis. This is an important one because shortcuts were very important to all of the participants."

If anything, we probably want to make it configurable per breakpoint once #1734642: Move breakpoint module into core hits.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

we probably want to make it configurable per breakpoint

Having shortcut sets as configurables #1497380: Convert shortcut sets to ConfigEntity allows us to store additional setting per set (number of items of breakpoints)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

Small clarification

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue summary: View changes
Status: Needs work » Fixed

No longer needs an upgrade path. Please open a new issue for follow-up.

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Title: Completely remove the ability to limit the number of shortcuts per set (upgrade path and followup) » Completely remove the ability to limit the number of shortcuts per set

Followup was in #1777392: $limit parameter removed from shortcut_admin_add_link, but still in documentation and is no longer relevant.

I'm sure it needs a D7-to-D8 migration path in theory, but let's leave that to the aether.