- Issue created by @dydave
- Merge request !165Issue #3537721 by dydave: Admin Toolbar Search: Removed dependency on Admin Toolbar Tools. → (Merged) created by dydave
- 🇫🇷France dydave
Quick follow-up on this issue:
All the changes detailed in the issue summary have been implemented and described in the merge request MR !165 above at #2.
Since all the tests and jobs still seem to be passing 🟢, moving issue to Needs review as an attempt to get more testing feedback and reviews.
Feel free to let us know if you have any comments, questions or concerns on any aspects of this issue or the suggested changes in the merge request, we would surely be glad to help.
Thanks in advance! - 🇩🇰Denmark ressa Copenhagen
Great idea to decouple the two sub-modules, and make them independent of each other -- nice job @dydave!
I can confirm that with the MR, the two sub-modules are now operating as totally individual sub-modules, and you can install and use one without the other.
Installing them together or separately, all features still work, such as Search without Tools, and vice versa, all features are still present.
If I try to install just one or the other, only Admin Toolbar is required, as expected:
$ drush install admin_toolbar_search The following module(s) will be installed: admin_toolbar_search, admin_toolbar
$ drush install admin_toolbar_tools The following module(s) will be installed: admin_toolbar_tools, admin_toolbar
- 🇫🇷France dydave
Thanks a lot @ressa, as usual for your promt and very positive feedback! 🙏
I'm really glad this idea made sense to you too and the changes worked as expected with your tests!
Honestly, I was a bit surprised to see this had not been done earlier, since it actually required very little code changes with few impacted files. 😅
So quite easy overall, but with a significant impact.Following your confirmation at #4, I went ahead and merged the changes at #5 🥳
I'm quite happy we could move forward quicker with this task, since it unblocks some of the work I have been doing recently, trying to clean-up and refactor the Admin Toolbar Search module.
I should be following up shortly with another issue for the Tests and the module file, then another one for the JS code. 👍
I thought it would be better to break down these refactoring changes into smaller/related changes with specific issues and merge requests, so it would be easier to revert or roll-back in the future.
Once again, thanks a lot for your great help @ressa reviewing and testing these issues as always.
Make sure to let me know if you spot anything specific that would need to be addressed quickly, I would surely be very happy to help. 🙂 - 🇩🇰Denmark ressa Copenhagen
You're welcome @dydave, I am glad I could assist you in improving Admin Toolbar! And it does seem like a slightly odd and limited dependency. But maybe in the past, they were much closer intertwined, and gradually they drifted apart, until the final break, today?
Your plan with breaking down the changes into bit-size updates for better management and easier roll-back if needed sounds like a great plan, and it's good to hear that this issue unblocks other areas of your plans for improvement. There's nothing worse than stalling, when you have momentum, and things are flowing. If too long time passes with no progress, you may forget the ideas and plans, and if far too long time passes, there's a risk of a complete halt in the process, and it never gets done.
So about helping each other out, I thought about a new feature a while ago, but think I didn't create an issue ... but now I finally did: ✨ Allow flagging erxtra important issues, to fast track completion Active . Feel free to add more suggestions for features, or other improvements if you see any 🙂