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.

CommentFileSizeAuthor
#47 2016-03-30 20_46_12-MINGW32__c_xampp_htdocs_drupal.png5.3 KBdbjpanda
#47 merge_list_shortcuts-2421011-46.patch21.83 KBdbjpanda
#39 merge_list_shortcuts-2421011-39.patch21.82 KBjibran
#36 merge_list_shortcuts-2421011-36.patch21.82 KBjibran
#36 interdiff.txt1.71 KBjibran
#35 shortcut-set-edit.png45.05 KBtstoeckler
#35 shortcut-set-add.png48.25 KBtstoeckler
#34 screenshot-s66b9473e1f0293f.s3.simplytest.me 2015-03-29 19-23-50.png25.98 KBjibran
#33 screenshot-s66b9473e1f0293f.s3.simplytest.me 2015-03-29 19-24-34.png22.8 KBjibran
#33 screenshot-s66b9473e1f0293f.s3.simplytest.me 2015-03-29 19-24-34.png22.8 KBjibran
#33 screenshot-s66b9473e1f0293f.s3.simplytest.me 2015-03-29 19-22-54.png43.72 KBjibran
#33 screenshot-s66b9473e1f0293f.s3.simplytest.me 2015-03-29 19-22-26.png26.62 KBjibran
#33 screenshot-s66b9473e1f0293f.s3.simplytest.me 2015-03-29 19-21-07.png17.51 KBjibran
#33 screenshot-s66b9473e1f0293f.s3.simplytest.me 2015-03-29 19-20-42.png8.83 KBjibran
#32 make_shortcut_add_and-2421011-32.patch20.54 KBjibran
#32 interdiff.txt1.25 KBjibran
#31 make_shortcut_add_and-2421011-31.patch19.5 KBjibran
#31 interdiff.txt677 bytesjibran
#30 make_shortcut_add_and-2421011-30.patch19.5 KBjibran
#30 interdiff.txt3.76 KBjibran
#26 screenshot-d8.dev 2015-03-23 22-26-54.png25.37 KBjibran
#26 screenshot-d8.dev 2015-03-23 21-28-47.png57.68 KBjibran
#26 screenshot-d8.dev 2015-03-23 21-23-42.png75.87 KBjibran
#26 make_shortcut_add_and-2421011-26.patch18.19 KBjibran
#26 interdiff.txt16.89 KBjibran
#24 shortcut-set-admin.png28.78 KBtstoeckler
#24 user-shortcut.png23.56 KBtstoeckler
#22 interdiff.txt3.86 KBkgoel
#22 2421011-22.patch3.78 KBkgoel
#13 better-shortcut-titles-2421011-13.patch2.22 KBAunion
#13 shortcut1Old.PNG10.65 KBAunion
#13 shortcut1.PNG10.91 KBAunion
#13 shortcut2Old.PNG15.02 KBAunion
#13 shortcut2.PNG15.41 KBAunion
#13 shortcut3Old.PNG8.68 KBAunion
#13 shortcut3.PNG9.12 KBAunion
#13 shortcut4Old.PNG9.23 KBAunion
#13 shortcut4.PNG9.85 KBAunion
#13 shortcut5Old.PNG9.07 KBAunion
#13 shortcut5.PNG11.3 KBAunion
#6 2421011-3.png1013.46 KBYesCT
#3 shortcut.2421011.3.patch836 bytesYesCT
#1 shortcut.2421011.1.patch1.41 KBYesCT
#16 shortcut6.PNG6.13 KBAunion
#16 shortcutOld6.PNG6.03 KBAunion
#16 better-shortcut-titles-2421011-15.patch3.12 KBAunion
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

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

YesCT’s picture

Status: Active » Needs review
YesCT’s picture

FileSize
836 bytes

actually. let's just do the page title and make the field label consistant in another issue.

idebr’s picture

  1. For consistency, let's use the same text for the the action link on the Shortcut overview page as the ShortcutController::addForm title. Currently the action link text is 'Add shortcut' while the addForm uses 'Add shortcut link'. The other way around could work as well, eg. use 'Add shortcut' for both.
  2. The title of the Shortcut overview page is currently 'List Links'. This is inconsistent with other overview pages, for example taxonomy uses 'Taxonomy' and node uses 'Content'. Perhaps this title should be updated to 'Shortcuts' consistent with the link on the configuration overview?

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

YesCT’s picture

YesCT’s picture

Issue summary: View changes
FileSize
1013.46 KB

so we can see what we are talking about

amateescu’s picture

Status: Needs review » Needs work

NW for #4.

tstoeckler’s picture

+1 for "Add shortcut" over "Add shortcut link".

jibran’s picture

Issue tags: +Novice, +Needs screenshots

This is confusing. We have ShortcutSet and Shortcut entities so to remove confusion:

  • We should change the title of entity.shortcut_set.collection route form 'Shortcuts' to 'Shortcut Sets' or 'List of Shortcut Sets'.
  • We should change the title of 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".
  • We should change the title of shortcut.link_add route from "Add shortcut" to "Add shortcut link".
  • We should change the title of entity.shortcut.canonical route from "Edit" to "Edit shortcut link".
  • We should change the title of entity.shortcut.edit_form route from "Edit" to "Edit shortcut link".
  • We should change the title of entity.shortcut.delete_form route from "Delete" to "Delete shortcut link".
  • We should change the title of 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.

idebr’s picture

@jibran In #8 @tstoeckler suggested to use 'Shortcut' over 'Shortcut link'. Can you confirm with him the text should be 'Shortcut link'?

jibran’s picture

@idebr I'm fine with Shortcut link see

We should change the title of shortcut.link_add route from "Add shortcut" to "Add shortcut link".
Aunion’s picture

Assigned: Unassigned » Aunion
Aunion’s picture

Assigned: Aunion » Unassigned
Status: Needs work » Needs review
FileSize
2.22 KB
10.65 KB
10.91 KB
15.02 KB
15.41 KB
8.68 KB
9.12 KB
9.23 KB
9.85 KB
9.07 KB
11.3 KB

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

jibran’s picture

Thank you @Aunion for the patch and screenshot it looks good now.

+++ b/core/modules/shortcut/shortcut.routing.yml
@@ -50,7 +50,7 @@ shortcut.link_add:
+    _title: 'Add shortcut link'

@@ -58,7 +58,7 @@ entity.shortcut.canonical:
+    _title: 'Edit shortcut link'

@@ -66,7 +66,7 @@ entity.shortcut.edit_form:
+    _title: 'Edit shortcut link'

@@ -82,7 +82,7 @@ entity.shortcut.delete_form:
+    _title: 'Delete shortcut link'

I think we should capitalize the word shortcut here.

We should change the title of shortcut.set_switch route from "Shortcuts" to "Shortcut Sets" same for the tab name.

I think you missed the tab name part of this :)

The last submitted patch, 13: better-shortcut-titles-2421011-13.patch, failed testing.

Aunion’s picture

Updated patch with changes mentioned in #14. Added screenshots of the change.

jibran’s picture

Thanks for the fixes. Now we can change the page title in tests as well

Message Group Filename Line Function Status
Page title 'List of Shortcut links | Drupal' is equal to 'List links | Drupal'. Other ShortcutSetsTest.php 46 Drupal\shortcut\Tests\ShortcutSetsTest->testShortcutSetEdit()

The last submitted patch, 16: better-shortcut-titles-2421011-15.patch, failed testing.

kgoel’s picture

Status: Needs review » Needs work
YesCT’s picture

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

kgoel’s picture

@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

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.78 KB
3.86 KB
jibran’s picture

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

tstoeckler’s picture

Status: Needs review » Needs work
FileSize
23.56 KB
28.78 KB

I 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 shortcut set switch form with red arrows annotating the senselessness of the plural usage
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?

jibran’s picture

Assigned: Unassigned » jibran

I'll fix the above review.

jibran’s picture

I have fixed #24.

  1. No more switching between shortcut link and shortcut everything is shortcut now even the entity label.
  2. Removed customize-form form shortcut set entity and list builder.
  3. Moved shortcut set customize-form to edit-form.
  4. Added add shortcut operation to shortcut set listing page just like menu listing page.

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.

dawehner’s picture

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

  1. +++ b/core/modules/shortcut/shortcut.routing.yml
    @@ -90,7 +82,7 @@ shortcut.set_switch:
       path: '/user/{user}/shortcuts'
       defaults:
         _form: 'Drupal\shortcut\Form\SwitchShortcutSet'
    -    _title: 'Shortcuts'
    +    _title: 'Shortcut set'
       requirements:
         _custom_access: 'Drupal\shortcut\Form\SwitchShortcutSet::checkAccess'
    

    Are we sure we want to change the string for the users?

  2. +++ b/core/modules/shortcut/src/ShortcutSetForm.php
    @@ -12,6 +12,8 @@
      * Form controller for the shortcut set entity edit forms.
    + *
    + * @property \Drupal\shortcut\ShortcutSetInterface entity
      */
     class ShortcutSetForm extends EntityForm {
    

    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?

  3. +++ b/core/modules/shortcut/src/ShortcutSetForm.php
    @@ -54,15 +129,36 @@ public function form(array $form, FormStateInterface $form_state) {
    +
    

    Let's drop that newline, while we are here.

  4. +++ b/core/modules/shortcut/src/ShortcutSetListBuilder.php
    @@ -33,12 +34,12 @@ public function getDefaultOperations(EntityInterface $entity) {
    +      $operations['add'] = array(
    +        'title' => t('Add shortcut'),
    +        'weight' => 20,
    +        'url' => Url::fromRoute('shortcut.link_add', ['shortcut_set' => $entity->id()]),
    +      );
    

    Nice idea!

jibran’s picture

Thanks for the review care to comment on

'Edit shortcut page' is missing 'Edit shortcut set' link in breadcrumbs.
  1. Yes it's wrong before.
  2. I didn't understand can you please explain.
  3. I'll fix it in next patch.
  4. I got that idea form menu listing page. ;-)

Status: Needs review » Needs work

The last submitted patch, 26: make_shortcut_add_and-2421011-26.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
3.76 KB
19.5 KB

Fixed test fails and #272 & 3.

jibran’s picture

Issue tags: -Novice
FileSize
677 bytes
19.5 KB

Minor fix.

jibran’s picture

Title: Make shortcut add and edit form have better title (and be consistent with others like menu link) » Merge "list shortcuts" page into "edit shortcut set" page and make shortcut add and edit form have better title
FileSize
1.25 KB
20.54 KB

Fixed the breadcrumb issue. This is ready for usability review.

jibran’s picture

jibran’s picture

tstoeckler’s picture

Issue summary: View changes
Issue tags: -Needs usability review
FileSize
48.25 KB
45.05 KB

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

+++ b/core/modules/shortcut/src/ShortcutSetListBuilder.php
@@ -33,12 +34,12 @@ public function getDefaultOperations(EntityInterface $entity) {
     if (isset($operations['edit'])) {
...
+      $operations['add'] = array(
+        'title' => t('Add shortcut'),
+        'weight' => 20,
+        'url' => Url::fromRoute('shortcut.link_add', ['shortcut_set' => $entity->id()]),
+      );
     }

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:

A screenshot showing the shortcut set add form and the user shortcut set form side by side pointing out the clash in titles with large red arrows.
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.

A screenshot of the shortcut set edit form pointing out the useless help text with a large red arrow captioned 'New set? I'm editing an existing one.'
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.

jibran’s picture

FileSize
1.71 KB
21.82 KB

Awesome work on this @jibran!!!

Thanks and thanks for the through review.

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.

This is same as menu_ui_system_breadcrumb_alter

I think this should be added outside of the if statement.

This is same as MenuListBuilder::getDefaultOperations()

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.

Fixed.

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.

Removed form edit page.

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.

I'm pretty much done here.
In #1340500-14: Merge "list terms" page into "edit vocabulary" page @xjm pointed out

Oh, I think maybe the "Add Term" button should also come between the vocabulary fields and the ordering sections; it's a little confusing at the top as it's away from the list of terms. Looking at https://www.drupal.org/files/Screen%20Shot%202013-02-01%20at%208.13.59%2... that was what was done in #663946: Merge "List links" page into "Edit menu" page as well.

Do you think we should move "Add shortcut" task below the text field?

Status: Needs review » Needs work

The last submitted patch, 36: merge_list_shortcuts-2421011-36.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
21.82 KB

Reroll and ping about the patch review.

hass’s picture

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes
Status: Needs review » Postponed
Related issues: +#2520232: Separate the menu settings from the 'add link' button

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

jonathanshaw’s picture

Issue summary: View changes
Status: Postponed » Active

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Title: Merge "list shortcuts" page into "edit shortcut set" page and make shortcut add and edit form have better title » Make shortcut add and edit form titles better
Issue tags: +Needs reroll

Let's just do the title changes here. There is still no consensus in #2520232: Separate the menu settings from the 'add link' button .

dbjpanda’s picture

Assigned: Unassigned » dbjpanda
dbjpanda’s picture

Re-rolled the patch from comment #39. please review.

Status: Needs review » Needs work

The last submitted patch, 47: merge_list_shortcuts-2421011-46.patch, failed testing.

The last submitted patch, 47: merge_list_shortcuts-2421011-46.patch, failed testing.

The last submitted patch, 47: merge_list_shortcuts-2421011-46.patch, failed testing.

xjm’s picture

Status: Needs work » Postponed

Let'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!

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.