Adjust Admin Toolbar settings page elements

Hi, I just reviewed and tested this locally on a D10.4.6 site. Everything works as expected and the new render arrays/ page layout make sense. Can't find any problems with spelling / words etc. +1 for RTBC

Thanks for reviewing @schillerm, feel free to change the Status :)

RTBC+1 updating status.

Thanks, though it's not clear to me if you tried the patch and evaluated the changes?

I ask because I see that you commented "Patch in #9 works fine, this has been verified by multiple developers, moving this to RTBC." in 🐛 PHP Fatal error: Declaration of Drupal\\flag_anon\\FlagAnonLinkBuilder::build($entity_type_id, $entity_id, $flag_id) Active ...

Changing status to RTBC requires that you apply the patch, and check if the changes make sense:

Reviewed & tested by the community ("RTBC")

[...]
Setting an issue to this status is a judgment call. It means you have thoroughly tested and reviewed the issue [my emphasis] and believe it is ready. If you are unsure, the status should not be changed.

From https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →

Thanks for the feedback!
I understand that I should have added more details before moving this to the RTBC.

I have reviewed the MR and tested on my Drupal 10.4.6 installation and here are the details of my testing:
Patch application: I downloaded patch from the MR and applied using git apply.

Testing:
Before applying the patch -

1. The order of the configuration fields was 1. Menu depth 2. Toolbar sticky behavior 3. Toolbar hoverIntent behavior
2. The order of the radio buttons was 1. Enabled 2. Disabled 3. Disabled: Hide on scroll-down, show on scroll-up
3. All the configurations was working properly.

After applying the patch -

1. configuration fields order is changed to 1. Toolbar sticky behavior 2. Toolbar hoverIntent behavior 3. Menu depth
2. The order of the radio buttons is changed to 1. Enabled 2. Disabled, show on scroll-up 3. Disabled
3. All the configurations is working properly after applying patch.

Additionally:
I verified that there are no coding standard issues (phpcs) introduced by these changes.
Also no new errors or warnings were observed in the logs.

Based on this review, I believe the changes in MR can be merged.

Thank you!

@ressa! 🥳
Thanks a lot for your great help keeping this show on the road!
I've been very busy with work on projects lately and couldn't find the time to reply earlier 😅

OK, I've taken a quick look at the merge request and it looks great!👍

While we're at it, I thought we could go a bit further and added the following two changes:

• Add a small text at the top to describe a little bit what the module does and what can be configured in the settings form.
• Move the menu depth field in some kind of "default" settings fieldset, or menu/toolbar related.

I didn't really "like" to see the "Menu depth" field "floating" in the form like that...
Additionally, based on your comments above @ressa, I assumed we could probably hide the setting by default, since it is not "very" likely to be changed.

As for all the wording, labels, texts, etc... these are more suggestions, at this point:
Feel free to edit any added textual elements as you would see fit 🙂

Overall, these changes would help us pretty much stabilize/wrap up all the previous changes made to this form, so they could be properly packaged in the next stable release 👍

I would greatly appreciate any feedback and reviews on the changes to the form (collapsed/open?), the help text, or anything else we could have missed.

Thanks again everyone for the help moving this issue forward! 🙏

Hi @dydave, great to have you back and moving the improvements along, awesome! I really like your suggestions for the expanded text descriptions, and they are perfect.

Actually, after making the MR and thinking more about it afterwards -- I also wasn't too happy about the "Menu depth" drop-down, and how it seemed to sort of get tacked on randomly at the end ... and, believe it or not, I even thought about perhaps putting the "Menu depth" drop-down under an "Advanced" fieldset, but then got away from it again. So great that you caught this detail, and actually did it 🙂

It looks ready to me, and very nice that we could get these final touches included, so let's move it along. It'll be great to get all the recent improvements released. Thanks!

Thanks a lot @ressa for the prompt and positive feedback! 🥳

Really happy I'm able to look at the module again and keep working with you on the next release 🤝

Following your confirmation above at #12, I did a last review of the merge request and since all the jobs and Tests seemed to be passing 🟢, I went ahead and merged the changes above at #13 🥳

Very good work in this issue, following-up with the recent changes that were made to the settings form and the added Tabs 👍
This is really helping make the module more presentable in the next release.

Marking issue as Fixed for now:
Feel free to create more issues if you see any further improvements that could be made to the form or anything we could have missed.
Otherwise, we'll make sure the form stays a bit maintained when adding more parameters or making changes in the future.

Thanks again to everyone for the great help testing and reviewing this issue! 🙏

Fantastic @dydave, and I agree we have a great co-operation with the recent additions to Admin Toolbar 🙂
It's so great to get yet another improvement ready and committed 🎉

PS. @rakesh.regar: Thank you for the very thorough review you provided. I was positive that I posted a "Thank you, now you should totally feel free to change Status to RTBC" comment, but can't see it now ... so maybe I just wrote it, but forgot to post it? But here it is anyway: Thank you so much for the very thorough review in your comment #9! It was great to get confirmation that the patch worked as expected, and did what it was intended to do, and that you even checked for coding standard issues (phpcs). I am so sorry, that I forgot to post the comment.

Automatically closed - issue fixed for 2 weeks with no activity.