Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#49 | 2395797-49.patch | 7.75 KB | eugene.brit |
#45 | 2395797-45.patch | 9.28 KB | yogeshmpawar |
#45 | interdiff-2395797-43-45.txt | 1.74 KB | yogeshmpawar |
#43 | 2395797-43.patch | 7.76 KB | yogeshmpawar |
#33 | 2395797-33.patch | 7.89 KB | yogeshmpawar |
Issue fork drupal-2395797
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
Elijah LynnComment #2
Elijah LynnComment #3
Elijah LynnBasically, I want to know if a patch would be accepted. If so I can roll it.
Comment #4
hass CreditAttribution: hass commentedThis module is a backport of D8 core. I think you need to get the feature in D8 core first.
Comment #5
msound CreditAttribution: msound commentedI understand that to have this merged I need to do this in D8 first. However, I thought I'd throw in the patch here anyway. This patch works with 7.x-1.x-dev.
Comment #6
hass CreditAttribution: hass commentedMoving for decision making to D8. Please move back once done or just close if we do not like to implement this at all.
Comment #7
xjmComment #8
Wim LeersThis is exactly what D8 already does.
Are you basically asking to do this always? Having used this myself for over a year, I personally think that that would make sense. But only for the "admin menu" toolbar tray, not for the other toolbar trays (e.g. shortcuts & user).
Comment #9
Elijah LynnYes, I am asking for it to always be on the left. Even when not on mobile. If I load in mobile then the menu floats to the left but if I am on desktop it floats to the top. I want it to float to left by default on mobile or desktop.
update: I agree that it should just be for the Admin Menu toolbar tray, not the shortcut and user.
Comment #10
Bojhan CreditAttribution: Bojhan as a volunteer commentedWe already made the decision in #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop. We decided for horizontal. The opinions on preferences around this are probably split 50/50. We made the decision to go for horizontal, I see no reason to revisit that decision.
Comment #11
temkin CreditAttribution: temkin as a volunteer commentedIf the opinions are split 50/50, wouldn't it be logical to allow site admins decide what should be the default layout. So far each client I showed this toolbar to asks to have it in vertical layout and that's the override I have to do and maintain for multiple sites.
I haven't looked into the module code yet, but I think it should be possible to move that setting into a variable, rather than having it hardcoded.
What do you guys think?
Comment #12
Juanpgs CreditAttribution: Juanpgs at Adaptive commented#5 It works fine!
Comment #13
xjmRetitling to that scope and moving to 8.2.x where features go. Thanks!
Comment #14
temkin CreditAttribution: temkin as a volunteer commentedHere is a patch that adds configuration form for Toolbar module that allows to set a default orientation - vertical or horizontal. I've tested it myself and seems to be working fine.
I must tell that JS logic probably needs to be refactored. It's pretty confusing that a lot of logic is tied to VerticalLock parameter. I think it should be more generic and rely on user selected orientation. If user hasn't selected anything, then it can fallback to a default setting.
Comment #16
temkin CreditAttribution: temkin as a volunteer commentedSeems like tests failed in the unrelated module. I've run tests in my local environment and they passed successfully. Marking for re-testing.
Comment #19
temkin CreditAttribution: temkin as a volunteer commentedI wanted to bring this to attention as I have a number of requests to have Toolbar with vertical orientation by default on both Drupal 7 and 8. As I understand before it gets to Drupal 7 it needs to be approved for 8 first. Can anyone review the above patch? Thanks!
Comment #20
Wim Leers@temkin: The Toolbar in Drupal 7 and 8 are vastly different. If you're referring to the D7 backport of the D8 toolbar, that's https://www.drupal.org/project/navbar, and that can happen any time.
Comment #21
temkin CreditAttribution: temkin as a volunteer commentedOK, thanks. I was under the impression that it needs to be implemented for D8 first and then backported to D7. Probably because of the comment #6. If that's not the case, I'll open a ticket in Navbar's module issue queue and submit a patch there. We can keep this issue specifically to D8.
Any chance it can get reviewed?
Comment #25
FranciscoLuz CreditAttribution: FranciscoLuz commentedToolbar module has gone through changes since patch from #14 was submitted.
Some bits and pieces of code have moved around and portions of what previously was residing in toolbar.js now lives in a file called toolbar.menu.js
Comment #27
jcmartinezWould it be possible to move this into its own module and fix it so that it woks with the latest version?
Or, add a configuration to the core to give administrators the choice?
Comment #28
cgomezgHi, I just upgraded the patch from #14 for 8.5.x
But is not working for me
Comment #29
cgomezgHi, I just add the real patch from #14 for 8.5.x
Admin path admin ../config/user-interface/toolbar
Comment #31
paper boyPatch from #29 works for me on a fresh Drupal 8.7.2
Comment #32
yogeshmpawarComment #33
yogeshmpawarRe-rolled the #29 patch because it's failed to apply on 8.8.x branch.
Comment #34
YahyaAlHamadOur company made a new design for the toolbar. If you would like to check it:
https://www.drupal.org/project/seeds_toolbar
it's not perfect, but it is vertical.
Comment #36
AlexBorsody CreditAttribution: AlexBorsody commented#29 works for me. Make sure you clear all caches.
Comment #40
dsdeiz CreditAttribution: dsdeiz at Skvare commentedComment #41
dsdeiz CreditAttribution: dsdeiz at Skvare commented#33 works for me.
Comment #42
dwwNeeds work b/c:
There’s a pile of issues trying to add toolbar settings. Eg
#2464193: Provide configuration options for toolbar menu
Tried to add that as related but I’m on my phone and it’s not working.
Point is: if it seems weird in here to add a whole new settings form for 1 knob, we’ll have a lot of other knobs we could add to it soon. 😉
Thanks,
-Derek
Comment #43
yogeshmpawarRe-roll of the patch against 9.3.x & keep it in Needs Work as it needs issue summary update.
Comment #44
yogeshmpawarComment #45
yogeshmpawarTrying to resolve CS issues.
Comment #49
eugene.britRe-roll for 10.0.x
Comment #52
rpayanmPlease review.
Comment #53
dwwTestbot can’t run the tests.