Add Show on scroll-up / Always hide option to Disable sticky toolbar

pls review

Nice, thanks @darkdim, that works perfectly!

I got an idea, to display the options like Development settings (/admin/config/development/settings) in a container, so I updated the MR. I could set it the issue RTBC, but someone else should probably do that.

I have added a title to the container. Pls review

Thanks for the update, I do think I prefer a fieldset and title around the sticky_options_wrapper, since it adds both 1. a title, 2. a border around the radio buttons, and 3. an indentation, to show it's a sub-element (which I added with "07686fc8 - Show options as fieldset"):

    $form['sticky_options_wrapper'] = [
      '#type' => 'fieldset',
      '#title' => $this->t('Disable sticky toolbar behavior'),

... over this, which only adds a title, and no indentation:

    $form['sticky_options_wrapper']['sticky_behavior'] = [
      '#type' => 'radios',
      '#title' => $this->t('Disable sticky toolbar'),

Right now, both are set, and it should probably only be one of them ... so I'll remove title under radio.

Sorry, about this, I shouldn't have updated the MR, but left a code review instead.

Slide or fade-in, when scrolling up

I thought about the way the Admin Toolbar re-appears, when you scroll up. And though it currently works well, I do think it would be nicer if the menu showed itself more gradually, like the way it slides the menu into view from above at https://www.1xinternet.de/en

It could also be a fade-in, where it's always there, but just transparent, and scrolling up makes it visible, if that's easier to code?

Do you think this would be possible? If it's difficult, the way it works now is all right, but I do think it would be the optimal solution with a more fluid transition, and I added this in the the Issue Summary:

It would be nice if the transition was smooth, i.e. the menu slides down from the top, when you scroll up.

Updated Issue Summary's "Proposed resolution".

Thanks a lot for the great work on this issue everyone!

I've taken a closer look at the initial MR and wanted to suggest a few changes, so I've created the new merge request MR!123 above at #11, with a few comments about some of the differences with the previous MR.

Could you please try giving this patch a round of tests and reviews?

The disable sticky behavior is still in DEV, in the sense it has not yet been published in a stable release, therefore, we "should" still be able to change the property/configuration, without necessarily requiring all the hook_updates/install code.
Therefore, removing/merging the 'Disable sticky' field into a different field integrating a more generic logic (moving from bool to string), shouldn't be a problem if that seems like a better/simpler choice for the form UI.

Re #8:
@ressa, I think it "should" be possible to do this with custom CSS, or JS in your theme.
I think the 'FadeIn' animation you mentioned could be achieved with Vanilla CSS transition animations on opacity, for example. Probably the same for the 'SlideIn' animation, with a position property.

We could always explore this further, but I wouldn't really see how custom animations could be integrated to the admin_toolbar module itself.
Ideally, this is something that could perhaps sit in a separate module, such as Animate CSS → , which seems to offer very comprehensive sets of animations that could be implemented with CSS classes (fade, bounce, slide, glide, etc...).
I'm not opposed to the idea or feature, I just think it would be very complicated to match the level of flexibility or customization you could obtain with a more specific library.
We could for example add a documentation example CSS file with different CSS rules implementing various animations, fade, slide, etc... Users could then copy/paste the rules in their themes and change all the parameters by themselves (speed, animation, levels, etc...).
Any other options... (?)

In any case, we would certainly be glad to hear your feedback on this new merge request and some of the suggested changes.
Thanks in advance!

@TODO: More tests should be added for the 'hide_on_scroll_down' option.

Example of 'FadeIn' animation on scroll-up with CSS only:

/* Admin Toolbar's sticky behavior to hide on scroll-down. */
body.sticky-toolbar-hidden.toolbar-fixed .toolbar-oriented .toolbar-bar {
  opacity: 0;
  /* Hide immediately */
  transition: none;
}

/* Admin Toolbar default visible state. */
.toolbar-fixed .toolbar-oriented .toolbar-bar {
  opacity: 1;
  /* Simple fade in animation on the 'opacity' property. */
  transition: opacity 400ms ease-in;
  will-change: opacity;
}

Paste this code in your theme CSS or the file from the merge request css/admin_toolbar.sticky_behavior.css for testing purpose and you should be able to see a similar animation to the site you indicated above.

Hope that helps!

This is awesome @dydave, thanks for carrying on the great work by @darkdim!

* Great new interface
I like the new user interface, having the options as a radio list, while clearly signalling the current (default) sticky behaviour on installation, and the options.

* Wording
The wording could be this, to signal that both option 2 and 3 are disabled, but different behaviours, by adding "Disabled: " to the last option?

• Enabled
• Disabled
• Disabled: Hide on scroll-down, show on scroll-up

* Fade-in
I added the fade-in CSS in css/admin_toolbar.sticky_behavior.css and it works great, thanks! Just a detail, perhaps the fade-in could be a little snappier, like 200ms?

Thanks @ressa for your help testing and once again for the very positive and constructive feedback 🙏

No problem for the changes to the wording, I'll make the changes to the MR a bit later 👌

For the Fade-in and animations, what's your take on this ?!
Do you see this as part of the module, or as I suggested earlier (#12)?
Keep it simple (bare minimum/no animation) and allow users to override the corresponding CSS rules with their own animations, based on documented CSS suggestions, for example.

To change the animation above (feel free to play with it @ressa):
See the CSS line above transition: opacity 400ms ease-in;
It is possible to modify this value to 200ms to make it a bit quicker + change potentially the type of transition, ease-in, ease-out, ease-in-out, or use multiple transitions, etc...
For more details, see: https://developer.mozilla.org/en-US/docs/Web/CSS/transition

See:

Just a detail, perhaps the fade-in could be a little snappier, like 200ms?

This is what I'm afraid about 😅
If everyone starts asking for different types of animations, delays, effects, etc.... It's not going to be viable for us 😅
That's why, I would be personally be more inclined to try to keep this type of feature out of the module and left at the charge of the site developers to refine and pick the exact type of display they would like manually by changing the CSS themselves.

I'm not against or opposed to anything, I'm just worried of the number of requests from users to have this type of feature adapted to their needs 😅
We could include this type of effect/transition in the module as an example that developers could override, but I don't see any type of customization of these types of transitions going into the module itself.... sounds like too much work 😅

Any feedback, advice, comment or suggestions would be very appreciated.
Thanks again very much for all your help @ressa!

Thank you @dydave for as always a fast and thorough answer!

Just to be clear: I don't think or suggest that any of these transitions or times should be configurable -- we just set some sane defaults, and allow the user to override them, if they so desire.

Also, approaching this purely from an "aesthetical" user experience POV, I think the fade-in CSS should be included, since with the current method, the reappearance seems to me too "abrupt". A gradual appearance is more pleasant. Don't you think, putting on the "regular-user"-hat?

Since I believe the majority would prefer fade-in, we should document how to disable the fade-in, for the minority, who don't want this. And about the speed, let's just set it to 200ms, and again -- if someone likes another speed or method, they should feel free to override.

It all sounds great to me @ressa!!

All good with all your points above 👍
I think we're on the same page in terms of the fancy bells and whistle animations 😅

we just set some sane defaults, and allow the user to override them, if they so desire.

Let's do it !

Feel free to keep posting reviews ... I'll address them later today, maybe in 5 hours or so....

Thanks again! 🙂

Yes, I agree, let's keep it simple and user friendly :)
I don't think I have more additions, these two were my only thoughts of changes ... thanks!

Thanks @ressa!

The merge request MR!123 has been updated with the suggested changes and seems to still be passing all tests 🟢

• Updated 'hide_on_scroll_down' radio field option label.
• Updated sticky behavior CSS file to support fadeIn animation by default.
• Added PHPUnit tests to check for the 'hide_on_scroll_down' libraries on page.

 
See the resulting Admin Toolbar Settings form:

At this point, this MR should pretty much be good to go, thus moving back to Needs review 👍

Additional comments, reviews and testing feedback would be greatly appreciated! 🙂
Thanks in advance!

ressa → changed the visibility of the branch 3509584-add-show-on to hidden.

Awesome work @dydave, and even getting the tests done as well -- and it looks great!

A last minor thing, is an occasional flicker, if I use the mouse wheel to scroll down, or press Down key, or Page Down key a few times -- I guess it's because the pixel value difference hits 1 pixel, detecting this as the user scrolling up, triggering a change of state -- even though as a user, this was not the action ...

Perhaps it's possible to make the scroll up/down detection less sensitive, and only react as the user scrolling up, if the change was more than 1 pixel and less than the movement when scrolling up or down in the window? It looks like a Down key press moves the screen around 55 pixels.

So perhaps the difference threshold could be set to something between 1 and 55 pixels, like 10 pixels?

PS. It can be a bit of struggle to trigger the occasional "false" fade-in ... I was tempted to set the issue to RTBC, and if you can't replicate it, we can set it to RTBC, and sort it out in a follow up issue. What do you think?

Re #21: I'll look a bit closer into that and do a bit more testing to see if there is anything we could do to improve the JS interactions, in particular try testing the solution you suggested 👍

Thanks again @ressa! 🙂

@ressa: I've done a bit more testing, but I'm not really seeing the issue with the scroll or key/up down:
It all looks pretty smooth to me 😅

I "think" I can understand what you mean, in terms of giving a bit of flexibility to the change on window, but I'm not sure I'm really able to replicate this with default settings (browser, drupal install, theme, modules, etc...).

I was tempted to set the issue to RTBC, and if you can't replicate it, we can set it to RTBC, and sort it out in a follow up issue. What do you think?

Indeed: I would tend to think the current patch/behavior "should" be stable enough to be included in a release, thus, setting it back to Needs review, hoping to get more reviews and feedback 🙂

Why not create a follow-up issue, if you'd like, for the behavior mentioned at #21, but maybe let's see if we could get users feedback, experiencing the same type of behavior (?)

It would be great if we could get a bit more testing feedback and reviews on this issue before it gets merged 🙏
Thanks again for your great help!

Thanks for checking, and yes -- it can be a chore to replicate :) ... I can always create a follow up issue, if it gets too much.

I have found a way of replicating the movement artificially for easier development, running scrollBy in Console, which scrolls the document up 2 pixels: window.scrollBy(0, -2);

If (when) a 10 pixel threshold for triggering a fade-in is implemented, this 2 pixel movement up shouldn't then make the admin toolbar reappear.

I am attaching a screen-capture of a real mouse scroll-wheel induced false positive.

From a functionality perspective I think it's RTBC, but comments on the code are welcome!

Thanks a lot @ressa, once again for the very clear description and video, it's greatly appreciated 🙂

No problem, we're going to look into this and see if we could find a proper solution 👍

I think I'm going to have to ask some help from other colleagues more specialized in front-end/JS development to see if they would have any advice on the way we could implement a threshold for the scroll animation.

Let's sit on this issue for a bit:
We've got as far as we could for now and we still have some time before the next release anyway.
I'd like to get at least 4 or 5 other tickets in before we release the next stable.... so we probably still have a few more weeks in front of us 👌
(see the recently updated tickets in Needs review, quite a lot of issues are moving🥳)

I'll take a little bit of time in the coming days to speak with some colleagues and try to put together something concrete that could be tested more carefully.
 

In the meantime, feel free to let us know know if you have any questions, concerns or comments on this particular issue, we would certainly be very happy to hear your feedback.
Thanks in advance!

You're welcome @dydave! I am very grateful for the thorough explanation and road plan, and that you will check in with your colleagues, it sounds very good :)

Great to hear that there are many issue in the pipeline! Perhaps it's time to consider a "[META] Plan for Admin Toolbar release 3.6" and list the issues there, prioritized under headers like for example "Important/low effort" and "Nice to have"? That way, us regular users can know which issues to focus on, for patch testing, and similar tasks.

Thanks @ressa!

I told my colleague Laurent about this issue, this morning and funny thing, he said he knew exactly what you meant 😆

He told me he would look at the code when he has some time and get back to me on this, if there is anything we could do 👌

Great idea for the META issue, we could create this next week maybe 👍

Hopefully... more to report back soon 🤞

Heh, nice to get this behaviour confirmed, and it sounds great if he can take a look at it, thanks! Perhaps we need a new word for this?

Back Scroll: When a mouse scroll wheel bounces slightly back in the opposite direction, after scrolling up or down.