Problem/Motivation
In /admin_toolbar/admin_toolbar_tools/src/Plugin/Derivative/ExtraLinks.php
there is a constant MAX_BUNDLE_NUMBER
which defines the maximum number of bundles (or sub-menu's) that will be generated - for example the number of sub-menus under Content Types or Taxonomy.
This is not documented, so it was not clear at first why the sub-menu's did not include all my content / taxonomy types.
Trying to manipulate the menu manually - the menu reacted unpredictably. Menu items manually added would be removed / changed.
Proposed resolution
Increase this value from 10 to 20, make the MAX_BUNDLE_NUMBER
a configuration option and at the very least document this.
Remaining tasks
- Add a configuration form for the module
- Add a configuration option for Maximum Number of Sub-Menu's
- Update the documentation: mention this setting
- Increase the default from 10 to 20
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff_52-54.txt | 8.42 KB | raman.b |
#54 | 3094835-54.patch | 24.32 KB | raman.b |
| |||
#52 | 3094835-52.patch | 17.09 KB | romainj |
#51 | 3094835-51.patch | 16.09 KB | romainj |
#47 | 3094835-47.patch | 17.64 KB | romainj |
Comments
Comment #2
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #3
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedKindly apply this patch , it works
Comment #4
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #5
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #6
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedNew patch for above issue
Comment #7
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedFinal Fixed added service dependency in patch
Comment #8
hash6 CreditAttribution: hash6 at QED42 commentedReviewed the above patch and added few changes pertaining to position of menus and clearing linting errors.Attaching the interdiff for the same.
Comment #9
hash6 CreditAttribution: hash6 at QED42 commentedComment #10
hash6 CreditAttribution: hash6 at QED42 commentedComment #11
hash6 CreditAttribution: hash6 at QED42 commentedComment #12
hash6 CreditAttribution: hash6 at QED42 commentedComment #13
hash6 CreditAttribution: hash6 at QED42 commentedComment #14
hash6 CreditAttribution: hash6 at QED42 commentedComment #15
hash6 CreditAttribution: hash6 at QED42 commentedAdded both
-- @hardik's code for bundle settings form
-- updated menu and cache fix for the form.
Comment #16
BerdirYou are missing default configuration and schema and an update function that sets the default, that's why tests are failing.
Comment #17
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #18
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #19
Tomazetti CreditAttribution: Tomazetti as a volunteer commentedAdded #16 and the missing form.
Comment #20
andypostMakes sense to provide "
configure: admin_toolbar_tools.settings
" to module's info.ymlcould be added more visibility on modules page
Comment #21
hash6 CreditAttribution: hash6 at QED42 commentedComment #22
idebr CreditAttribution: idebr at iO commentedAttached patch implements the following changes:
Comment #23
DuneBL#22 failed on last dev:
Note also that it could be interresting to change the padding of the menu items because when MAX_BUNDLE_NUMBER is higher then a lot of item are hidded.
see the attached small patch to achieve this.
Comment #24
romainj CreditAttribution: romainj as a volunteer commentedAs discussed in different issues like https://www.drupal.org/project/admin_toolbar/issues/2908747 we do not want to have any configuration for the Admin Toolbar module. We wish to keep it plug and play.
As for the documentation I will add some explanation on the module page.
Comment #25
DuneBL@romainj ok, what about increasing MAX_BUNDLE_NUMBER and decreasing the padding like in my patch?
For example, If you use bootstrap_paragraph, by default you will not see all the paragraph type which is not very handy
Comment #26
andypost@DuneBL then it sounds better to have as theme variable (same config under the hood)
Comment #27
romainj CreditAttribution: romainj as a volunteer commentedAs we're having more and more complains about the number of items to display, we're going to make it a state value. So that number will be an option for each instance of a site and not a configuration to deploy.
I will provide a patch later on.
Comment #28
idebr CreditAttribution: idebr at iO commented#27 Can you elaborate why it should be state, rather than configuration? When using configuration synchronisation, the configuration is identical over different environments, so the preferred amount of items to display will be identical as well.
Comment #29
romainj CreditAttribution: romainj as a volunteer commented@idebr I think that the number of items to display depends on the size of your screen and it's not really a configuration of the site. On a project each developer would be able to choose how many sub-menus they want to appear. Moreover it is more a visual option than a feature option.
Comment #30
idebr CreditAttribution: idebr at iO commented#29 Sure it is dependent on the size of your screen, but what if you are looking at the same site? Visual configuration is also configuration, a typical example is Views configuration.
Comment #31
BerdirI'm also not sure how state would be better here, if it's really something per user/developer then it would need to be local state/storage in the browser like some other things, but this is something that's decided at the time of route building I think, so you can't have different users with different settings on the same site? At best you could visually limit it further down from the configured setting.
Comment #32
alisonAm I understanding correctly -- "state" would be like the "show/hide row weights" thingy all over the Drupal UI?
(If yes) For this number-of-bundles-to-show situation, is the idea that it would be a similar toggle, like, "show all"/"show 10" (or whatever the default max number is)?
And, would it be for each entity type, or would it be a site-wide toggle (like row weights)?
I get the principle of the module being plug-and-play, but isn't it sufficiently plug-and-play in that you don't *have* to do anything after turning on the module? (Or is it that exporting/codifying a config entity is "doing something"? If so, I can understand that, it definitely is "doing something," just trying to understand.)
One way or another, I desperately want to be able to see all my ctypes in the admin menu. It drives me crazy I don't have access to as many menu levels in D8 as I did in D7, I'm still miffed about that haha -- so when I could no longer get to all my bundles from the admin menu, I was devastated all over again!
Thanks, all :)
Comment #33
JasonSafro CreditAttribution: JasonSafro as a volunteer commentedDear Admin Toolbar Team,
Thank you for making a great module. It seems like you put in a lot of work and made something that is very useful. I've been wondering for months why my menu items kept disappearing. I finally sank the time in today to backtrack it to the MAX_BUNDLE_NUMBER constant.
I love that your module is plug-and-play. But, I hate that I can't configure it. It seems like a lot of people feel the same way. At least 20 users took time to backtrack the issue to the constant and comment on here; I bet hundreds more users are experiencing the same problem but don't know what to do about it.
It sounds like you're being very flexible and trying to make some changes; that's great! But, your solution seems pretty complicated. What about a really simple approach:
This feels like a really simple approach that would make people happy and is easy to implement.
Best,
Jason
Comment #34
webdrips CreditAttribution: webdrips commentedNicely said @JasonSafro; I agree it would be nice to have the option to increase this.
I did increase it in my distribution to 20, and decreased the padding a bit for submenu items, and can now see how 20 will easily fit now (but the menu is still quite readable IMO).
Comment #35
DuneBLIf there are too many submenu to fit in the window we should be able to scroll down to see the hidded submenu. This is not the case right now.
Comment #36
romainj CreditAttribution: romainj as a volunteer commentedPatch #23 has also display bug.
Comment #37
romainj CreditAttribution: romainj as a volunteer commentedI fixed the patch #22.
Comment #39
romainj CreditAttribution: romainj as a volunteer commentedComment #40
DuneBLLooks like the form is missing.
Here is what I get after applying the #39 patch:
Comment #41
romainj CreditAttribution: romainj as a volunteer commentedComment #42
DuneBLI confirm #41 apply correctly and is doing the job.
Some idea:
1-When reaching first time the config form, there is no default value... I think we can add one
2-Create a setting to choose a small or a large padding:
Depending of the setting a class could be added to the
<a>
tag3-Much more complicated: allow to scroll down the menu
Comment #45
romainj CreditAttribution: romainj as a volunteer commented@DuneBL yes there is a default value but you need to run the
admin_toolbar_tools_update_8201()
function first (drush updb
).Comment #46
DuneBLI have tried to
updb
but I got the following:PHP Fatal error: Cannot redeclare admin_toolbar_tools_update_8201() (previously declared in .../modules/contrib/admin_toolbar/admin_toolbar.install:22) in .../modules/contrib/admin_toolbar/admin_toolbar_tools/admin_toolbar_tools.install on line 20
function exists twice
Comment #47
romainj CreditAttribution: romainj as a volunteer commentedComment #48
DuneBL#47 apply cleanly and runs has expected; I have tested the db update and the default value is set to 20
Comment #50
DuneBLAnother thing I am thinking about: the label
Maximum number of bundles per entity type to display
is a very technical description; this setting could be changed by a user who don't know anything about bundles. More over, I think this setting slice other menus...What not using something like
Maximum number of menu
? (or sub-menu)Comment #51
romainj CreditAttribution: romainj as a volunteer commentedComment #52
romainj CreditAttribution: romainj as a volunteer commentedNow the Admin Toolbar Search module has a dependency to the Admin Toolbar Tools module due to the use of the configuration settings.
Comment #53
DuneBL#52 failed on last dev:
Comment #54
raman.b CreditAttribution: raman.b at OpenSense Labs commentedResolving the failed test cases from #52
Comment #55
romainj CreditAttribution: romainj as a volunteer commentedPatch #54 works for me. Thanks @raman.b
Comment #56
webdrips CreditAttribution: webdrips commented#54 Worked for me thanks
Comment #57
jpdippenaar CreditAttribution: jpdippenaar commentedAs the person who originally logged the request - I do apologise for all the effort; I tested this and it is working perfectly for me. Thank you.
Comment #59
romainj CreditAttribution: romainj as a volunteer commentedCommited into the lastest dev version. Thanks to all.
Comment #60
romainj CreditAttribution: romainj as a volunteer commented