Issue #3484575 "Move the edit button outside of more actions dropdown"
MR has been created against #3484575 and below tasks have been done:
- Two new icons for "Kebab" and "Edit"(AKA Pencil) have been added to the top bar.
- "More Actions" text has been replaced with the Kebab menu icon.
- Edit button has been moved out of the more actions dropdown and is prefixed with the pencil icon.
- Styling changes have been added.
- The menu_local_tasks Twig override has been implemented, along with the removal of some existing code that previously removed the local tasks. As a result, the secondary menu tabs, which were not visible earlier, are now displayed. (@lauriii @ckrina - This implementation is per what we discussed.)
- Top Bar functional test has been updated.
Below are some of the screenshots of how UI looks after the above changes:
Node View Page:
Secondary local tasks.
Merge request reports
Activity
added 135 commits
-
a84a102c...1c773fed - 130 commits from branch
project:11.x - 345994dc - 3484575-Adding-Icons
- 1353ce3e - 3484575-Moving-The-Edit-Action-Out-Of-Top-Bar
- c405dfeb - 3484575-Styling-Changes
- 49f3638d - 3484575-Updating-Functional-Test
- a61adf62 - 3484575-Refactoring-Code-And-Adding-Primary-Local-Tasks-Condition
Toggle commit list-
a84a102c...1c773fed - 130 commits from branch
48 48 // Local tasks for content entities. 49 49 if ($this->navigationRenderer->hasLocalTasks()) { 50 50 $local_tasks = $this->navigationRenderer->getLocalTasks(); 51 52 $edit_route_name = key(array_filter( 53 $local_tasks['tasks'], 54 fn($task) => isset($task['#link']['#title']) && $task['#link']['#title'] === 'Edit' 
That doesn't work.
For starters, there are translations you have to consider, on a german site, it won't be Edit, but "Bearbeiten". You could assert the translatable string, but that too isn't reliable.
I think it's much more reliable to assert on the route name, which should end with ".edit_form". That's not guaranteed, but it's a defacto-standard, especially for all entity types that use the default html route provider.
-
@berdir I see. Thanks for explaining how the text Edit could be problematic. Routes indeed seem to be the only reliable way to determine if the task is an edit task.
Initially, I worked with route names, but they varied across entities. In my testing with media, nodes, taxonomy terms, and users, some routes ended with .edit_form, while others ended with .canonical. I will take a look at routes again.
-
@berdir I had a quick look across at the "edit" page route_names of the below entities:
- Node edit page route: entity.node.edit_form
- User edit page route: entity.user.edit_form
- Media edit page route: entity.media.canonical
- Taxonomy Term edit page route: entity.taxonomy_term.edit_form
- Block Content edit page route: entity.block_content.canonical
- Menu Link edit page route: entity.menu_link_content.canonical Note: There could be other entities as well.
Do you think we should write conditional logic on route names against respective entities to achieve this? OR Something else exists that could help in route identification?
-
Interesting. The cases that you mention are all special.
First, I assume that we only want to show a visible edit link if you are viewing that entity. Not when you are already editing, or deleting or are on any route other than canonical for said entity type? You're already doing something then, the primary action for you to go to is likely not edit. For Edit, my understanding is that we'll then show the save button there instead.
- Media: local task actually uses edit form. This one is conditional, because by default, you can't view medias standalone. but there is a setting and then canonical and edit_form are different things and edit on canonical should work.
- block_content: You can't view them, only edit (and delete and translate). So we don't need to show edit
- menu_link_content: Same, you can only delete, you can't view a standalone link.
https://www.drupal.org/project/drupal/issues/3498127 I have created another issue regarding replacing the edit link with save link.
changed this line in version 11 of the diff
204 * from display to avoid duplicating the links. 205 * 206 * @param array $build 207 * A renderable array representing the local_tasks_block plugin block to be 208 * rendered. 209 * @param \Drupal\Core\Block\BlockPluginInterface $block 210 * Block plugin object representing a local_tasks_block. 211 * 212 * @see navigation_block_build_local_tasks_block_alter() 213 */ 214 public function removeLocalTasks(array &$build, BlockPluginInterface $block): void { 215 if ($block->getPluginId() !== 'local_tasks_block') { 216 return; 217 } 218 if ($this->hasLocalTasks() && $this->moduleHandler->moduleExists('navigation_top_bar')) { 219 $build['#access'] = FALSE; 
Humm, where specifically did you see that this was an issue? Are there content entity routes with secondary tabs? The local tasks should only be removed on content entity routes, per
\Drupal\navigation\NavigationRenderer::meetsContentEntityRoutesCondition.In any case, I can see how this could be necessary. I'm wondering if this should be its own issue...
Created https://www.drupal.org/project/drupal/issues/3500162 to determine how to address this scenario.
Could you please remove changes related to it from this MR?
changed this line in version 13 of the diff
38 38 $variables[$region->value] = $element[$region->value] ?? NULL; 39 39 } 40 40 } 41 42 /** 43 * Implements hook_preprocess_HOOK(). 44 */ 45 function navigation_preprocess_menu_local_tasks__navigation(&$variables): void { 46 $navigation_renderer = \Drupal::service('navigation.renderer'); 47 $variables['top_bar_tasks_exist'] = $navigation_renderer->hasLocalTasks(); changed this line in version 13 of the diff
47 48 display: flex; 48 49 gap: 0.5rem; 49 50 51 .toolbar-button-exposed { 52 position: relative; 53 padding: 0; 54 list-style: none; 55 color: var(--admin-toolbar-color-white); 56 background-color: var(--admin-toolbar-top-bar-action-color); 57 58 &::before { 59 position: absolute; 60 left: 10px; -
Lets use CSS logical properties (eg.
inset-inline-start). Also should be 12px according to Figma. changed this line in version 10 of the diff
47 48 display: flex; 48 49 gap: 0.5rem; 49 50 51 .toolbar-button-exposed { 52 position: relative; 53 padding: 0; 54 list-style: none; 55 color: var(--admin-toolbar-color-white); 56 background-color: var(--admin-toolbar-top-bar-action-color); 57 58 &::before { 59 position: absolute; 60 left: 10px; 61 mask-size: 90% auto; -
I think we're better off overriding
inline-sizeandblock-size, otherwise we're bound to the icon size. Figma has this icon size at 16px x 16px. changed this line in version 10 of the diff
54 list-style: none; 55 color: var(--admin-toolbar-color-white); 56 background-color: var(--admin-toolbar-top-bar-action-color); 57 58 &::before { 59 position: absolute; 60 left: 10px; 61 mask-size: 90% auto; 62 } 63 64 .toolbar-button__label { 65 z-index: 1; 66 } 67 68 .toolbar-dropdown__link { 69 padding-right: 16px; changed this line in version 10 of the diff
56 background-color: var(--admin-toolbar-top-bar-action-color); 57 58 &::before { 59 position: absolute; 60 left: 10px; 61 mask-size: 90% auto; 62 } 63 64 .toolbar-button__label { 65 z-index: 1; 66 } 67 68 .toolbar-dropdown__link { 69 padding-right: 16px; 70 color: var(--admin-toolbar-color-white); 71 padding-inline: var(--admin-toolbar-space-40); -
We want
padding-inline-startto be the larger value, butpadding-inline-endto be the smaller. See Figma. Values should be:Need to also define
var(--admin-toolbar-space-36). changed this line in version 10 of the diff
99 97 ], 100 98 ]; 101 99 $items['menu_region__footer'] = ['variables' => ['items' => [], 'title' => NULL, 'menu_name' => NULL]]; 100 $items['menu_local_tasks__navigation'] = [ 101 'template' => 'menu-local-tasks--navigation', 102 'path' => \Drupal::service('extension.list.module')->getPath('navigation') . '/templates', changed this line in version 10 of the diff
added 1 commit
47 48 display: flex; 48 49 gap: 0.5rem; 49 50 51 .toolbar-button-exposed { -
- i would better combine that button as group of modifiers. EG
button--bluebutton--size--smallor something. - use
--for modifier. - Don't increase specificity here.
.top-bar .toolbar-button-exposed {}don't have any sence here.
Edited by Ivan Berdinsky - i would better combine that button as group of modifiers. EG
changed this line in version 20 of the diff
54 list-style: none; 55 color: var(--admin-toolbar-color-white); 56 background-color: var(--admin-toolbar-top-bar-action-color); 57 58 &::before { 59 position: absolute; 60 inset-inline-start: var(--admin-toolbar-space-12); 61 inline-size: var(--admin-toolbar-space-16); 62 block-size: var(--admin-toolbar-space-16); 63 } 64 65 .toolbar-button__label { 66 z-index: 1; 67 } 68 69 .toolbar-dropdown__link { changed this line in version 20 of the diff
1 <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> 2 <rect width="16" height="16" fill="white" fill-opacity="0.01"/> -
The Kebab menu icon from Figma design, and pencil icon was shared by @lauriii
changed this line in version 20 of the diff
1 {# 2 /** 3 * @file 4 * Claro theme implementation to display primary and secondary local tasks. changed this line in version 11 of the diff
added 1 commit
45 48 'contexts' => ['route'], 46 49 ], 47 50 ]; 51 48 52 // Local tasks for content entities. 49 53 if ($this->navigationRenderer->hasLocalTasks()) { 50 54 $local_tasks = $this->navigationRenderer->getLocalTasks(); 55 $current_route_name = \Drupal::routeMatch()->getRouteName(); changed this line in version 13 of the diff
45 48 'contexts' => ['route'], 46 49 ], 47 50 ]; 51 48 52 // Local tasks for content entities. 49 53 if ($this->navigationRenderer->hasLocalTasks()) { 50 54 $local_tasks = $this->navigationRenderer->getLocalTasks(); 55 $current_route_name = \Drupal::routeMatch()->getRouteName(); 51 56 52 $edit_route_name = key(array_filter( 53 $local_tasks['tasks'], 54 fn($task) => isset($task['#link']['#title']) && $task['#link']['#title'] === 'Edit' 55 )); 57 // Canonical routes for supported entity types. 58 $canonical_routes = [ changed this line in version 13 of the diff
63 ]; 56 64 57 65 $exposed_local_tasks = []; 58 if (isset($edit_route_name) && array_key_exists($edit_route_name, $local_tasks['tasks'])) { 59 $exposed_local_tasks[] = [ 60 'task' => $local_tasks['tasks'][$edit_route_name], 61 'icon' => 'edit', 62 ]; 63 unset($local_tasks['tasks'][$edit_route_name]); 66 67 // Check if the current route is canonical. 68 if (in_array($current_route_name, $canonical_routes, TRUE)) { 69 // Look for the "Edit" task in the local tasks. 70 $edit_route_name = key(array_filter( 71 $local_tasks['tasks'], 72 fn($task) => isset($task['#link']['#title']) && $task['#link']['#title'] === 'Edit' changed this line in version 13 of the diff
added 99 commits
-
ede08a7d...1630acb8 - 98 commits from branch
project:11.x - 9c31f176 - Merge remote-tracking branch 'origin/11.x' into 3484575-refactor-top-bar-actions
-
ede08a7d...1630acb8 - 98 commits from branch
added 1 commit
5 5 * 6 6 * Available variables: 7 7 * - local_tasks: Array of local tasks for the current route. 8 * - featured_local_task: The local task to show featured before the dropdown. 8 9 */ 9 10 #} 10 11 {% set dropdown_id = 'admin-local-tasks'|clean_unique_id %} 12 13 {% if featured_local_task %} 14 {# The featured_local_task.task is a render array and the text passed to the parent 15 component needs to be an string. Hence we are rendering the text and passing 16 it to the component.#} 17 {% include 'navigation:toolbar-button' with { changed this line in version 26 of the diff
added 9 commits
-
dc890350...0be56730 - 7 commits from branch
project:11.x - f3caecfd - Merge remote-tracking branch 'origin/11.x' into 3484575-refactor-top-bar-actions
- 447c3435 - Issue #3484575: Refactor Edit butoon SDC implementation
-
dc890350...0be56730 - 7 commits from branch
11 11 {% set dropdown_id = 'admin-local-tasks'|clean_unique_id %} 12 12 13 13 {% if featured_local_task %} 14 {# The featured_local_task.task is a render array and the text passed to the parent 15 component needs to be an string. Hence we are rendering the text and passing 16 it to the component.#} 14 {% set link = featured_local_task.task['#link'] %} 17 15 {% include 'navigation:toolbar-button' with { 18 text: featured_local_task.task|render, 16 text: link['#title'], changed this line in version 26 of the diff
added 1 commit
added 7 commits
-
c5940699...54c4637e - 5 commits from branch
project:11.x - cea665f2 - Merge remote-tracking branch 'origin/11.x' into 3484575-refactor-top-bar-actions
- 0366311b - Issue #3484575: Make plugin class internal
-
c5940699...54c4637e - 5 commits from branch
added 1 commit
added 1 commit
added 1 commit
- 2a412512 - Remove large-icon variant since it's not defined
added 1 commit
