Toggle away Admin Toolbar completely

I have checked the patch is working fine. Please check the below images.
Before applying the patch, when we open non-admin routes the "Toggle button" is not visible on the left top bar.

After applying the patch, when we open non-admin routes the "Toggle button" is visible on the left top bar. The button toggle functionality is also working fine.

The patch does not work properly for me on D9.4.5 and PHP 7.4.

On admin routes the toolbar looks like this:

On Non-Admin routes the toolbar shows like this:

On Non-Admin routes when the toggle button is clicked the toggle icon shows like this:

The toggle works properly but the background color of the toggle icon breaks.

Changing to Needs work

@vinaymahale I have updated the code. Please check the new patch.

Thank you @Shyam_Bhatt for the patch.

Before testing the patch, I'm setting it to "Needs work" so that the patch is re-submitted with a proper naming convention.
I encourage you to kindly have a look at the naming convention for a patch.

Here is a reference link for you to go through:
https://www.drupal.org/project/naming/git-instructions

@vinaymahale Please check the below patch:

admin_toolbar__toggle_complete_toolbar-3304810-11.patch

Thank you @Shyam_Bhatt for the patch.

I tested the toggle functionality. It works well, but there needs more enhancement.
The toggle remains open if I navigate to any menu links on the non-admin routes.

Steps to reproduce:
1. Install the module on an existing Drupal 9.x. site
2. Apply the patch from comment #11
3. Visit a non-admin route
4. Toggle to hide the admin toolbar
5. Then, Open a menu link of the site from the non-admin route
6. The toggle opens up and the admin toolbar is visible
7. Expected: the toolbar should not be visible since it was hidden in step 4

Changing to "Needs work"

Once the enhancement is done, can proceed with testing

@vinaymahale Once the page reloads the Toggle button will be in the initial state.

@Shyam_Bhatt By initial state, do you mean the admin toolbar remains open after every page reload (on the non-admin routes)?
That is how it works for me after applying the patch.

@vinaymahale Yes, The admin toolbar remains open after every page reload (on the non-admin routes).

Patch #11 applied successfully on on Drupal 9.4.5 and PHP 8.1.6.
The patch does not work properly for me.
After applying the patch I tested the toggle functionality.It works well,but i get symbolise at the place of arrow.
Refer to screenshots.

Any updates or work done here?

Really good and helpful idea! Especially when theming projects and you need to be logged in, but the admin toolbar should be toggled away. In mobile devices it's quite space-consuming.

So yes, would be nice to proceed here.
As a first step, the patch might be turned into a MR for easier review and merge?

Thanks, might also go into https://www.drupal.org/project/gin_toolbar - what do you think makes more sense? Or should you ask the admin_toolbar maintainers first, if they are interested in this feature?

+1 for this feature! I've wanted that for a long time for the same reason as the author of this issue.

Great! Until now, we have always had to solve this by creating a new role especially for this scenario, which is an unnecessary effort.

Thank you very much for your work. But I am a bit confused. The toolbar on the left disappears, but:
- The padding on the left remains
- The toolbar at the top of the screen remains

Just the icons of the toolbar on the left are gone. I currently have zero benefit. The goal would be to be able to hide the admin areas in the frontend as if you were viewing the website anonymously. But the admin bars still disrupt the whole layout.

@sascha_meissner thank you very much, it's working now.
When hidden and when reloading, the page jumps because the toolbar is displayed first. This is due to the nature of the thing when the script is executed, so probably not much can be changed. In any case, a big step forward, thanks again.

Thanks! Perhaps the "Implementation" section could include an image or two, based on the latest MR?

Thank you for fast feedback @sascha_meissner, it's greatly appreciated. And I understand, things can get in the way. I have added the remaining tasks in the Issue Summary.

There is also the new feature of letting the menu stay at the top, which may help in some cases.

Nice thanks! The first contribution is always special :)

Did some minor adjustments. Generally works great and definitly helps! Great work @sascha_meissner!

Someone should take a final look at the CSS changes and test it with the gin theme active.

Furthermore, I am unsure if the current key combination of "ctrl + alt + h" will suffice for toggling the toolbar. Maybe we should add a small button somewhere, which toggles the toolbar?

At last, we should discuss, whether the current key combination feels the most DX friendly and document the key combination somewhere (probably in the tooltip of the toggle button)

EDIT:
Ah, and:

Have an indicator that your toolbar is currently not displayed [...].

(#48)

Alright, I changed the key combination to "Alt + h" (since the Key combo for admin toolbar search is "Alt + a" I thought "Alt + h" might be more in line.

Furthermore, I implemented the collapse / expand button. @thomas.frobieter will now begin for the final styling + remaining fixes.

After that, we can do a final DX discussion.

Thanks for all the efforts here! I have a few comments:

• In Firefox, Alt + h opens the Help menu item, so maybe Alt + Ctrl + h is necessary?
• Instead of "Toggle", could it be branded as a "Hide shortcut"? It is then immediately clear what the feature does. "Toggle" is ambiguous, and could mean for example switching between a Light and Dark mode.
• Since this affects the entire Admin Toolbar, and to keep things simple (I am not sure the maintainers prefer a new module ... @dydave: What do you think?), maybe -- instead of in a new module -- we can instead add it as an option on the Admin Toolbar settings page (/admin/config/user-interface/admin-toolbar) below the new "Toolbar sticky behavior" and "Toolbar hoverIntent behavior" settings, which were just added ...

  Perhaps something like this?

  [ ] Enable Hide shortcut
  Adds a keyboard shortcut (Ctrl + Alt + h) to hide and show the Admin Toolbar.

  As an ordinary user of Admin Toolbar, I would personally prefer to not get yet another "Admin Toolbar" module. Also it would make the list of modules under "Extend" (/admin/modules) even longer ...

• Can we use capitalized function keys, like we do for Admin Toolbar Search shortcut (i.e. Alt + Ctrl + h) and on the Module shortcuts page?

Okay, I've added CSS + icons. I've also tested and fixed Gin, with its four toolbar styles.

Moved the .toolbar-expand-floating-button into body, because Gin hides #toolbar-administration in some situations.

I haven't checked smaller viewports / mobile yet.

I think we should not use hide(), show() and toggle() as function names in the JS file, as all three are jQuery functions, which is confusing. My naming is certainly not perfect yet either.

I am not sure about the settings.initial_toolbar_padding_top / settings.initial_toolbar_margin_left. What is the purpose of these settings? I am not a big fan of manipulating styles in JS. If possible, we should set classes instead and put the required CSS in admin_toolbar_toggle.css.

EDIT: Another note - due to the old CSS float technique, we cannot easily add the Hide Trigger button as the last element (visually). There are several outdated frontend techniques used by the admin toolbar. Gin has already fixed all of them, maybe it is a good idea to apply these styles some day.

Restoring "Needs discussion" tag, sorry @Grevil :)

I thought it was a left over tag, but now see it was added yesterday ...

Thanks a lot everyone for the great work on this issue, with the CSS, JS and icons! 😍
It really looks great, thanks a lot!

Based on the work in the previous merge request, I created the new merge request MR !146 above at #61 which suggests a different approach for the implementation of this feature:

The shortcut keys were changed in this MR 😅
How about Alt + p? Could that work for you?
I have a plugin/extension in my browser locally that is using Ctrl + Alt + h already...
So I thought we could maybe try with "Alt + p" ?

For the setting, I thought we could do something similar to #3494646: Access Admin Toolbar search field via keyboard shortcut Alt + a and add a setting in the recently added Advanced section of the settings form.

Most of the work in MR !146 is around the CSS and JS logic:
Again, I thought we could maybe try to capitalize/build on top of what was done in #3509584: Add Show on scroll-up / Always hide option to Disable sticky toolbar and re-use the display/hide animation and states/classes that were already defined.
The JS code was refactored to use Vanilla JS, fix ESLINT errors and implement a logic toggling CSS classes in the 'body' tag of the page, based on the local storage.
All the CSS styles and images for the toggle links were moved from the previous merge request.👌

Unfortunately, in this first draft, support for the Gin Theme had to be dropped: 😖
I noticed the 'hide_on_scroll_down' sticky behavior doesn't work very well with the Gin theme as well, so I thought maybe we could potentially address the issues at the same time.
Additionally, I "think" there is already quite a lot in this MR, so perhaps support for Gin for this feature should be part of a follow-up issue, maybe with a bit more generic approach? 😅

I would greatly appreciate to have your feedback and reviews on this new merge request, in particular, the shortcut key, the wording, labels, textual elements in general, the CSS/JS code and approach, etc...
Thanks in advance!🙂

Thanks @thomas.frobieter and @dydave! I agree with the solution in the latest MR, to include the feature inside Admin Toolbar module, and not as a separate module, to keep things simpler.

I also noticed the Gin-specific code, and handling Gin-specific solutions in a follow up issue seems like a good idea.

Perhaps the new feature could be placed on same level as "Toolbar sticky behavior" and "Toolbar hoverIntent behavior", below them, and not under "Advanced settings"?

Instead of "Toggle", it could be branded as the less ambiguous "Hide shortcut" to make it immediately clear what the feature does. "Toggle" is ambiguous, and could mean for example switching between a Light and Dark mode. The option could then be something like [ ] Enable Hide shortcut (Alt + p) ... But if everyone agree that "Toggle" is best, that's also totally all right :)

Everything looks very good and clean to me now!

Alt+P works fine for me. I've added it to the title text of the toggle buttons!

Additionally, I "think" there is already quite a lot in this MR, so perhaps support for Gin for this feature should be part of a follow-up issue, maybe with a bit more generic approach?

+1 for this.

Hide vs. Toggle

About renaming from "Toggle" to "Hide", maybe it's too much work? I think we should at least visit the subject, and then decide if it's too much effort ...

My reasoning is this: "Toggle" means changing the state of something (like I already commented, see #63).

Think about the Link weight button. It is using these words in the GUI:

• "Show row weights"
• "Hide row weights"

.. in the code you see this: tabledrag-toggle-weight-wrapper

But the button doesn't say "Toggle", right?

Advanced

Also, I do think moving it up a level from "Advanced" could be considered (again, see #63).

Thanks a lot @ressa as usual for the great help and feedback!

Thanks also Thomas (@thomas.frobieter) for the prompt and positive feedback on this updated merge request.

@ressa: No problem at all for all the suggested changes! 🙂

I have just moved the 'enable_toggle_shortcut' checkbox from the "Advanced" to the "Toolbar sticky behavior" ✅
It certainly makes sense, at least from a code perspective, since the setting is using the same CSS as the 'hide_on_scroll_down' sticky behavior.

I have also updated the checkbox label to: Enable keyboard shortcut (Alt + p) to show/hide the toolbar and couldn't find any more toggle keyword in the visible textual elements.

Should we also change toggle everywhere else? Or could we still at least keep it, as it is currently, for the variable names and such?
See for example, the name of the config: enable_toggle_shortcut

I think it "should" be fine to maybe keep it for variable names and in the code in general, as long as it is not visible by users and identifies clearly the feature for developers.

Otherwise, I've noticed we're using 'Alt + p' (lower case) in the form, but 'Alt +P' (upper case) in the JS code, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/146/di...
Are we fine keeping both? Or should we standardize with one of the two?

Feel free to send us more feedback, reviews and comments!
Thanks in advance! 🙂

Pure awesomeness @dydave, that's much better!

How about slimming down the label to this?

[ ] Hide or show the toolbar with shortcut (Alt + p)

I think, that toggle can stay "behind the scene" for example in variable names, it's the user facing part that's important.

Though the config names perhaps should perhaps be updated from enable_toggle_shortcut to enable_hide_show_shortcut, since you could argue that config is also a sort of user facing component ... Or is it easier to update everything from xyz_toggle_var to xyz_hide_show_var? I'll let you be the judge :)

About case, since we are using "Alt + key" (lower key) everywhere else, I think we should also use "Alt+p" here.

Thanks again @ressa for the very clear and speedy reply!

OK, I've dealt with the simple changes, with the checkbox label and "Alt+p" (lower case) ✅

Concerning the variable name, this is a much bigger change 😅
enable_toggle_shortcut
Indeed, the keyword toggle is in all the names of the CSS/JS files, classes, functions and/or variable names in the JS code, etc... 😅

Besides, I "think" the keyword "toggle" in terms of code, in this particular context, should be "legit", in the sense, it actually toggles (add/remove) CSS classes in the body tag of the page, from one state (visible) to another (hidden) and vice versa.
This is what the JS function toggle actually does:
https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle

Maybe we could change it to something like toggle_display_shortcut or toggle_toolbar_shortcut?
Or if you think hide_show_shortcut could work better, then I would also be fine with that 👌

But in any case, if we were to make this change, we would better be 100% sure of the new name, since this is a pretty impactful change that could break quite a lot of things in this MR 😅

Moving back to Needs review for more feedback!
Thanks again! 🙂

Other questions:
1 - What about existing sites?
With the current MR, when existing sites upgrade to this feature, it will be disabled by default (since the setting does not exist, has not been initialized), until the settings form is saved.
Should the feature be enabled or disabled after upgrade?
@TODO: Missing implementation of hook_update

2 - Test with Sticky behavior disabled:
When hiding the toolbar, the icon stays fixed on screen, when the toolbar itself stays at the top of the page.
Do we want to make this behavior consistent by having the expand icon also stay at the top of the page (instead of following the scroll)?

Thanks @dydave, for fast updates! The update label reads great now, it's immediately clear what it does.

To update toggle, or not: It's totally fine as it is then. The config variable is a minor detail, and probably understandable by most users. So let's keep toggle behind the scene :)

Behaviour with Sticky: I am fine with the current behaviour, but you could argue that it should slide away as well, if "Disabled, show on scroll-up" is selected. But I think it is fine as it is, especially if it takes a lot of work to tweak just right.

Enabled/Disabled/hook_update: This feature could be considered fairly niche (I will probably never use it) so maybe it should actually be disabled by default? This would also mean that there is no need for a hook_update. I believe this is the last outstanding task to consider. (As a side note, the shortcut conflicts with a browser add-on I use.)

Sounds great @ressa! 👍

Thanks a lot once again for keeping this feature on track 😅

Indeed, if you still find any textual element related with this feature that should be changed, we would surely be glad to do so.
Otherwise, let's keep the variable names as they are for now 👌

I have just updated the merge request to:

• Disable the 'toggle_shortcut' feature by default: Added a hook update to add the config variable value to module's config (a bit cleaner) and disabled the feature on install by default.
• Fix the expand button position for the disabled sticky behavior: Added the expand button CSS selector to the existing disable sticky CSS styles.
• Remove unused svg files, copied from the previous merge request and related with support for Gin. We can surely reuse these files in another merge request to fix compatibility with the Gin theme.

 
Updated the Functional Tests accordingly and they all seem to still pass 🟢

As a side note, the shortcut conflicts with a browser add-on I use.

Not sure what we could do for the shortcut though 😅
Would you have any suggestions of keys or ideas?

Other than that, I did another overall review of the code of the merge request and any potential conflict with other features, and at this point, we should be OK 👌

Back to Needs review for more testing, reviews and feedback on the latest changes.
Thanks in advance!🙂

Hi @dydave and right back -- thanks for implementing the feedback so fast, 21 May is getting closer 😅

Your updates look great, and the shortcut is disabled on install by default. Nice touch that the icon is now also sticky, if "Disabled" is selected. And great with the clean up, it is always a good idea to remove unused code, and that tests are still green!

I think the current shortcut is fine, it's a custom one I defined myself. At some point in the future, it could be considered to offer a predefined list of shortcuts in a drop-down, so that it would be selectable, where "Alt + p" could be the default, for example these?

Alt + p
Alt + h
Ctrl + Alt + p
Ctrl + Alt + h
Shift + Alt + p
Shift + Alt + h

But a drop-down feature is probably best saved for next version, so this feature is RTBC 🚀

PS. A tiny detail, which perhaps could be fixed here as well when committing, is that perhaps "Toolbar: sticky" could be updated to just "Toolbar sticky" (or something like that) without the colon symbol, in this string?

The following settings mostly provide advanced configuration of the JavaScript behavior of the Toolbar: sticky and hoverIntent.

@dydave I tried applying the patch but it didn't worked out.
I tried on Drupal v10.4.8.

@jatingupta40 The patch applied fine when I tried it, as it should do, when it says "MR !146 mergeable". That literally means that the patch applies with the latest dev-version of the module ...

This is the method I use, and as the last command shows, there are no error messages, showing that the patch was applied:

$ composer require drupal/admin_toolbar:3.x-dev@dev
$ cd web/modules/contrib/admin_toolbar/
$ wget https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/146.diff
$ patch -p1 < 146.diff 
patching file admin_toolbar.install
patching file admin_toolbar.libraries.yml
patching file admin_toolbar.module
patching file config/install/admin_toolbar.settings.yml
patching file config/schema/admin_toolbar.schema.yml
patching file css/admin_toolbar.disable_sticky.css
patching file css/admin_toolbar.toggle_shortcut.css
patching file images/icons/collapse.svg
patching file images/icons/expand.svg
patching file js/admin_toolbar.sticky_behavior.js
patching file js/admin_toolbar.toggle_shortcut.js
patching file src/Form/AdminToolbarSettingsForm.php
patching file tests/src/Functional/AdminToolbarSettingsFormTest.php

Thanks

@ressa, thanks a lot for the very constructive and quick feedback, it's super appreciated! 🙏

Looks like we're going to have to be careful not to keep issues RTBC for "too" long in the issue tracker...
Otherwise they just might get abruptly merged 😅

Sorry I didn't have the time to make the suggested changes for the wording of the settings form before the MR got merged 😅

No worries: "Luckily" the 3.x build pipelines are now failing with PHPCS errors 🥳
So I'll make sure the changes to the form are added in the next MR to be merged asap to fix the dev branch build pipelines 👌

Otherwise, once again, I personally think you have great UX and feature ideas: I like very much your suggestion of a dropdown list of suggested shortcuts to make it configurable 👍 (efficient, simple, clear)
We could immediately create this ticket (feature) and probably group it/do the same kind of implementation for the Search focus shortcut (#3087173: Allow shortcut keys to be configurable). Some tickets, in particular feature requests could already start getting planned for the following release.

Additionally, we still have a spin off/follow-up ticket to create for this one ==> Improve compatibility with the Gin theme.
We definitely want to keep the work from @thomas.frobieter specifically related to supporting GIN, mostly the CSS and JS files (MR!70).
We'll most likely end up adapting/keeping most of the code, probably organized a bit differently.

So that's already 3 or 4 new tickets on which we could start working for the following release 👍

Lastly, thanks a lot for taking the time to kindly explain to other contributors/users how to apply patches or help testing module's merge request and for all the kind words and encouragements in other issues.

Special thanks to @sascha_meissner for getting this issue started and @grevil and @thomas.frobieter for helping getting it over the line!🙏

raaaaa.... looks like it was a random build fail 😅

Retrying the PHPCS job came back green 🟢
So the pipelines actually seem fine.

Let's try to make this change to the form wording in the next MR 👌

Awesome! Thanks, @dydave and @ressa for your quick back 2 back finalization of this! 😍

Thanks @dydave for all the great work here in Admin Toolbar, you're doing a stellar job! And yes, that was a bit of an abrupt merge ... 😅

It would have been great to be able to do the final small tweaks in this MR, but it's a good idea to include them in the next MR

Thank you for the nice words about my suggestions, I just try to approach it from up above, and see it from the perspective of a regular user (such as myself) -- like, are the headings and features easily understood at a glance, is the interaction user friendly, etc.?

I know how you can easily focus on getting the code working, while forgetting about the user interface. In #3498980: Add date-only date formats, and use in Announcements Feed, it was only at the last minute, after @penyaskito luckily noticed that the order of dates could be improved, with better sorting, that it got fixed ... I had overlooked that they were jumbled up, since my focus was on navigating the GitLab UI, writing (unused) updates_hooks, etc. 🙂

So building Drupal is a team effort, where we all take on different roles at different times, and help each other out, however we can.

I like your idea about merging the shortcut issue into a "Meta" issue, where we can deal with both "Admin Toolbar Search" and the new "Hide Toolbar" feature. I have updated the shortcut issue, so it is now aimed at both the Search and Hide Toolbar features. It's great that there are new features in the pipeline for future versions of Admin Toolbar!

Thanks @sascha_meissner for sharing your idea and code, and @grevil and @thomas.frobieter for pushing this feature forward! It's very encouraging with the friendly and positive approach from for example DROWL.de developers and @dydave, where the tone can occasionally be a bit too focused on efficiency in some issues, with hardly a thank you or gratitude. So I really appreciate the positive tone in the many projects you are maintaining, and issues you are involved in.