Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2416987: Fix UI regression in the menu link form found (and had code to fix)
that the shortcut add and edit form title would be better if it used the word shortcut on them.
Proposed resolution
Use shortcut in page title
Merge "list shortcuts" page into "edit shortcut set" page to improve usability
Remaining tasks
Implement tweaks from #2520232: Separate the menu settings from the 'add link' button
User interface changes
Yes.
It'll merge "list shortcuts" page into "edit shortcut set" page
API changes
No more shortcut links overview page.
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedthis takes some bits out of #2416987: Fix UI regression in the menu link form
needs issue summary update to do beta evaluation (see instructions in summary)
Comment #2
YesCT CreditAttribution: YesCT commentedComment #3
YesCT CreditAttribution: YesCT commentedactually. let's just do the page title and make the field label consistant in another issue.
Comment #4
idebr CreditAttribution: idebr commentedI already typed this about the fields before your #3 comment, so this comment is only here for archiving until there is a followup :)
The labels for the add/edit form fields are not mentioned in the issue title, but since they are included in the patch: let's make these consistent with the Menu UI add/edit form, for example 'Shortcut link title': 'The text to be used for this link'. The 'path' field needs a description as well, are there any path fields currently in core? The description should reflect the path can be an alias as well as a system path.Comment #5
YesCT CreditAttribution: YesCT commented#2421037: make field labels consistant for menu link and shortcut which both use the link widget
Comment #6
YesCT CreditAttribution: YesCT commentedso we can see what we are talking about
Comment #7
amateescu CreditAttribution: amateescu commentedNW for #4.
Comment #8
tstoeckler+1 for "Add shortcut" over "Add shortcut link".
Comment #9
jibranThis is confusing. We have ShortcutSet and Shortcut entities so to remove confusion:
entity.shortcut_set.collection
route form 'Shortcuts' to 'Shortcut Sets' or 'List of Shortcut Sets'.entity.shortcut_set.customize_form
route from 'List links' to "List of Shortcut links" or "List of {shortcut set name} Shortcut links" i.e "List of Default shortcut links".shortcut.link_add
route from "Add shortcut" to "Add shortcut link".entity.shortcut.canonical
route from "Edit" to "Edit shortcut link".entity.shortcut.edit_form
route from "Edit" to "Edit shortcut link".entity.shortcut.delete_form
route from "Delete" to "Delete shortcut link".shortcut.set_switch
route from "Shortcuts" to "Shortcut Sets" same for the tab name.This is a novice task and we need before and after screenshots for Usability purpose.
Comment #10
idebr CreditAttribution: idebr commented@jibran In #8 @tstoeckler suggested to use 'Shortcut' over 'Shortcut link'. Can you confirm with him the text should be 'Shortcut link'?
Comment #11
jibran@idebr I'm fine with Shortcut link see
Comment #12
Aunion CreditAttribution: Aunion commentedComment #13
Aunion CreditAttribution: Aunion commentedNew patch implementing changes from #9. Added before (end with Old) and after screenshots that illustrate the changes. Could not find the spot where Delete/"Delete shortcut link" shows.
Comment #14
jibranThank you @Aunion for the patch and screenshot it looks good now.
I think we should capitalize the word shortcut here.
I think you missed the tab name part of this :)
Comment #16
Aunion CreditAttribution: Aunion commentedUpdated patch with changes mentioned in #14. Added screenshots of the change.
Comment #17
jibranThanks for the fixes. Now we can change the page title in tests as well
Comment #19
kgoel CreditAttribution: kgoel commentedComment #20
YesCT CreditAttribution: YesCT commentedI dont think we should capitaize that.
I dont see another place in core where we do.
For example:
ag "title: .Edit "
shows:
core/modules/contact/contact.routing.yml
29: _title: 'Edit contact form'
core/modules/content_translation/content_translation.permissions.yml
7: title: 'Edit translations'
core/modules/field_ui/field_ui.routing.yml
45: _title: 'Edit view mode'
85: _title: 'Edit form mode'
core/modules/menu_ui/menu_ui.links.task.yml
2: title: 'Edit menu'
core/modules/menu_ui/menu_ui.routing.yml
20: _title: 'Edit menu link'
and others.
Comment #21
kgoel CreditAttribution: kgoel commented@YesCT mentioned to specify what needs work, here it is -
$this->assertTitle(t('List links') . ' | Drupal');
Change the title to match 'List of Shortcut links' in ShortcutSetsTest.php line 46
Also, agree with https://www.drupal.org/node/2421011#comment-9666269
Comment #22
kgoel CreditAttribution: kgoel commentedComment #23
jibranThanks @YesCT for the suggestion I am fine with capitalization we can always change it in CSS so no big deal but if it's inline with core then that's awesome. Thanks @kgoel for the changes and test fix.
Now to get this issue ready we need screen shots and beta evaluation in issue summary. It is ready from coding point of view.
Comment #24
tstoecklerI still find shortcut to be a preferable term over shortcut link. A shortcut set is a set of shortcuts, so the terminology does make sense together with shortcut set.
Regardless, there are two minor problems with the current patch as illustrated by the two screenshots:
The user switch shortcut set form should not be titled Shortcut sets because the form is about choosing one so the plural does not make sense.
The local action on the shortcut set admin form is Add shortcut which is inconsistent with everywhere else where it's shortcut link. As stated above I would like to see it changed to shortcut everywhere but at the very least it should be consistent.
I also have a major problem with the current Shortcut admin, which is not introduced here but IMHO makes sense to fix here as well: Just like we did for menus in D8 we should merge the Edit shortcut set name and the List links form into one form. That would get rid of the weird List links Dropbutton action alltogether. Thoughts?
Comment #25
jibranI'll fix the above review.
Comment #26
jibranI have fixed #24.
Shortcut set edit-form
Shortcut set add-form
I ran into one issue though
'Edit shortcut page' is missing 'Edit shortcut set' link in breadcrumbs.
I'll ping @dawehner about it.
Comment #27
dawehner+1 for making things more consistent with the way how menus work. IMHO its quite a WTF, because users expect the links to be under the edit form all the time.
Are we sure we want to change the string for the users?
Given that $this->entity is actually documented already on the parent class, don't we want to do a
/** @var \Drupal\shortcut\ShortcutSetInterface*/\n protected $entity;
instead?Let's drop that newline, while we are here.
Nice idea!
Comment #28
jibranThanks for the review care to comment on
Comment #30
jibranFixed test fails and #272 & 3.
Comment #31
jibranMinor fix.
Comment #32
jibranFixed the breadcrumb issue. This is ready for usability review.
Comment #33
jibranFresh round of screenshots.
User Home
User Shortcut set page
Shortcut set listing page
Shortcut set edit page
Shortcut add page
Shortcut edit page
Comment #34
jibranComment #35
tstoecklerAwesome work on this @jibran!!!
I will look into the breadcrumb thing. The custom implementation seems fine, but I don't yet understand why the breadcrumb doesn't just work out of the box.
I found one minor nitpick:
I think this should be added outside of the if statement.
I tried the patch out on simplytest.me and it looks great, especially that it so much resembles the menu administration.
I found two minor UX issues, however:
The inline help from
shortcut_help()
on the shortcut set overview form and the shortcut set add form mention the Shortcuts tab on the account page. The tab (=== local task) is now named Shortcut set, however.When I am editing a shortcut set,, I am notified about what happens when a shortcut set is created, which is not at all useful information at this point. I'm not sure but I think this is a pre-existing issue, but IMO makes sense to fix here.
Removing the Needs usability review tag for now. If we're done here in terms of code and "consistency review" I will post updated before-after screenshots so that the usability team can review this more easily.
Comment #36
jibranThanks and thanks for the through review.
This is same as
menu_ui_system_breadcrumb_alter
This is same as
MenuListBuilder::getDefaultOperations()
Fixed.
Removed form edit page.
I'm pretty much done here.
In #1340500-14: Merge "list terms" page into "edit vocabulary" page @xjm pointed out
Do you think we should move "Add shortcut" task below the text field?
Comment #39
jibranReroll and ping about the patch review.
Comment #40
hass CreditAttribution: hass commentedComment #42
xjmMoving to 8.1.x per https://www.drupal.org/core/d8-allowed-changes#minor and postponing on the outcome of #2520232: Separate the menu settings from the 'add link' button, which tries to address a usability regression from making a similar fix to menus.
Comment #43
jonathanshaw#2520232: Separate the menu settings from the 'add link' button has resolved with a reasonable generic solution for these cases. Ideally this case should use the same solution.
Comment #45
jibranLet's just do the title changes here. There is still no consensus in #2520232: Separate the menu settings from the 'add link' button .
Comment #46
dbjpanda CreditAttribution: dbjpanda commentedComment #47
dbjpanda CreditAttribution: dbjpanda commentedRe-rolled the patch from comment #39. please review.
Comment #51
xjmLet's postpone this issue on an overall issue to design and validate better UIs for these configure/list/etc. admin UIs that have multiple actions. See #2520232: Separate the menu settings from the 'add link' button for a starting point. We can incorporate the discussion from this issue as well, but we need someone's help to summarize the overall issue so the discussion does not continue to fragment.
Thanks!