Problem/motivation
Drupal 7 has most global site and page elements hardwired into templates, and gives no control to users to move them around. The primary and secondary link placements are hardwired to core and contrib themes alike. We should stop special-casing these, so they can be moved around and selectively hidden or replaced as needed.
Additionally, this causes code duplication, prevents efficient caching and causes a confusing site builder experience (/admin/structure/menu/settings
is not exactly discoverable).
Proposed solution
- Primary and secondary menus as new regions in the default
page.html.twig
and in the default (Bartik) theme. - The site builder can then put any menu block in the "Primary menu" region and it'll be styled as the primary menu. Similar for the "Secondary menu" region.
- All the special snowflake stuff can be removed:
the obsolete code inmenu.inc
the hacky code incommon.inc
to associate the cache tags for the primary and secondary menus, necessary because they're rendered in a backward way
the global menu settings (the route, the form, the config schema, the config file) to choose "THE primary menu" and "THE secondary menu".
- The goal: look identical to HEAD, which looks like this:
-
- With this patch: mission succeeded.
-
API changes
- The primary and secondary menus are converted into blocks, and therefore no longer available as theme settings or as variables in the page template.
- The following API functions are removed:
function menu_main_menu()
menu_secondary_menu()
_menu_get_links_source()
menu_navigation_links()
- The primary and secondary menu settings are removed from
menu_ui.settings
andtheme.settings
. - Two new regions in Bartik:
primary_menu
andsecondary_menu
.
Comment | File | Size | Author |
---|---|---|---|
#231 | Screen Shot 2014-09-28 at 3.33.22 PM.png | 10.27 KB | webchick |
#228 | global_menus_as_blocks-1869476-226.patch | 51.61 KB | Wim Leers |
Comments
Comment #1
Gábor HojtsyGiven #1535868: Convert all blocks into plugins, it probably does not make much sense at this point to convert these to blocks as blocks exist pre-plugins. However, @EclipseGC actually did lots of the conversion work that is involved with these blocks in interim patches at #1787846: Themes should declare their layouts. It was not actually related to introducing layouts as a concept, and I want to make sure we have the patch around, so here it is. All the code included is from @EclipseGC.
Blocks included:
- primary links
- secondary links
Technically this issue does not make much sense until #1535868: Convert all blocks into plugins however, I think it would make sense to parallelise work.
I don't agree with the ways the theming / markup changes are done in plugins (and the way themes need to override them), it is not my code, I just wanted to save it for @EclipseGC and start a discussion :)
Comment #3
andypost+1 to this.
I see this implementation different. @EclipseGC proposed some time ago a discovery for menus like ctools does for now.
While I involved in #1814916: Convert menus into entities I'd like to mention about inconsistency in menus - we have 2 different kinds of menu menu_list_system_menus() and menu_get_menus()... suppose if we break BC then each menu-edit form should have a checkbox "Provide a block"
Comment #4
EclipseGc CreditAttribution: EclipseGc commentedBlocks as plugins will already provide menus as blocks. Primary/Secondary menus are specially styled menus that are currently only usable at the page tpl level.
This patch is specifically breaking those elements out into reusable blocks, and providing bartik specific overrides (as bartik actually does do different styling for these).
Comment #5
dcrocks CreditAttribution: dcrocks commentedSince 'primary' and 'secondary' links are expressions of 'main' and 'user' menu, aren't they already blocks? If bartik would have them assigned to regions during install, as other blocks are, the static code in page.tpl wouldn't be needed. And since bartik's block/region mapping is given to all new themes when enabled, these would also have them by default. I can see Seven's need for the static code in page.tpl but I really don't see the need in any other theme.
As to 'special' styling, all menus are styled to meet design needs for every theme, as bartik already does. It seems simpler to me to assign User and Main menu to regions in bartik during install than to add code to make something a block when it already has a block implementation.
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedDrupal has long had a primary and secondary menu theme element. This is not new, we are not proposing anything new, we are simply porting what already exists. Also Primary and Secondary are actually pointers to other menus that can be configured (including pointing both to the same menu allowing for secondary displaying sub menu items for a given parent. These are obviously similar to menu blocks, but not the same. Unification of some sort might ultimately be wise, but likely not at this time. We need to get the existing feature set ported, not debate how to rewrite the feature set.
Eclipse
Comment #7
sunNote:
- Primary + secondary links have lost their meaning over years.
- The way Bartik outputs them is more akin to modern themes.
- The actual output of those menu links is not special; they are menu link trees starting on a certain level and limited to 1 level.
- The actual difference to other menu links is a special context/data-source: Secondary links render the 2nd level of links of a menu tree, but only if the corresponding parent link of the 1st/parent level is active.
- That special behavior is the default, but it can be re-configured on the Menu settings page.
Please also note that one of my primary goals for D8 was #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables, since Menu Block module replaces the entire primary/secondary links plus all menu blocks with configurable menu block instances, which are able to solve a dozen of different real world use-cases.
However, work on that issue has stalled, since it requires a notion of block instances, which is supposed to be introduced by Blocks & Layouts, and which I did not want to (temporarily) duplicate. But as soon as that is available, moving Menu Block's functionality into core and killing primary/secondary links altogether is what we actually want and need to do.
I'd suggest to do the following:
1) Change the default plugin implementation to a MenuLinksPlugin, which just renders a single menu with a depth limited to 1, and which gets the following plugin settings injected:
- menu_name to render
- the starting level to render (0 or 1)
- (id + class) HTML attributes to apply to the rendered output
2) Make what Bartik is doing (separate menus) the default, so it doesn't need to override it. Stark doesn't care, Seven doesn't output these links to begin with.
That will get us prepared for the further settings we want to add for menu block plugin instances and a good step closer to ditching the special concept of primary/secondary menu links ASAP.
Lastly, I also want to note that I'm a little scared that the existing patch here tries to implement plugins in a theme... First of all, I don't think we can expect this level of familiarity with PHP from themers, so that rings some alarm bells from my perspective. ;) I'm confident that we need different and more easy ways for themers to override things, which is probably a discussion topic for the plugin system of its own. Secondly, Drupal does not handle theme extensions in a reliable way currently, which essentially means that they can come and go at will. AFAIK, the plugin system needs reliable plugin implementations.
Comment #8
dcrocks CreditAttribution: dcrocks commentedI wasn't saying eliminate primary and secondary links. I am saying that bartik and many other themes don't need them. Those who do can still add the static code, or whatever evolves in drupal 8, to their templates and have the special behavior of these menus. I just think that code is unnecessary and disturbing to newcomers in bartik, which is the 1st theme that newcomers look at. It is unnecessary to convert them to blocks, just make them available by default to themes, as there is no way for a new theme to assign a block to a region before it is first enabled.
Comment #9
andypostSuppose if we make #1814916: Convert menus into entities in then menu module could introduce this kind of block instance plugins. The only question in context of menu entities - what module should hold the implementation of menu entities.
The menu module could just make UI to manage menus and their blocks
EDIT related #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()
Comment #10
Gábor HojtsyI propose we group at #1053648: Convert site elements (site name, slogan, site logo) into blocks first which is much simpler, but still needs to set up the pattern of introducing the blocks, new regions as needed, removal of existing theme variables, blocks added in install profiles, etc. That pattern can be replicated here and in #507488: Convert page elements (local tasks, actions) into blocks but both this one and the breadcrumb/message one need more discussion as to what we do with associated specific problems, while the logo/slogan and site name could work flawlessly.
Comment #11
dcrocks CreditAttribution: dcrocks commented#503810: Convert primary and secondary menus to blocks is an antecedent of this issue and has some interesting reading. But it is not a duplicate. Shouldn't it be resolved 1st?
Comment #12
Gábor Hojtsy@dcrocks: seems to me like this one and that one are clearly duplicates?! What would be the difference?!
Comment #13
dcrocks CreditAttribution: dcrocks commentedThat one advocates getting rid of primary/secondary links. If that were so, then this would simply about assigning 'main' and 'user account' menus to bartik during install.
Comment #14
dcrocks CreditAttribution: dcrocks commentedRead the patch, and I don't think the consensus there was as stated in #60.
Comment #15
Gábor Hojtsy@dcrocks: my reading of #503810: Convert primary and secondary menus to blocks is that there is no consensus; also there were only 2 posts the whole of 2012 and the last one before that was in 2011 March. I think that is safely closed as a duplicate. There is no consensus in here either what to do. :)
Comment #16
dcrocks CreditAttribution: dcrocks commentedThat sounds very reasonable. But if the primary/secondary link code is not going to be removed does anything need to be done? I can use 'Main' and 'User Account' menu blocks in my themes now, but it would be easier if Bartik also used them so that the usage would be 'inherited'. I would think Stark(through system page.tpl) would still use primary/secondary links.
Comment #17
Gábor HojtsyYes, if a specific block for primary and secondary menus is not introduced, we still need to remove the theme settings related to this. The menus should be displayed and used through blocks. Even in Seven/Stark or any other theme. No special theme variables anymore, that is the whole point of the blocks and layouts initiative.
Comment #18
EclipseGc CreditAttribution: EclipseGc commentedHow about something along these lines? I did not comment/test anything here, this is just a gut reaction to the current state of things. As all theme components are moved to blocks the preheader region I added to bartik can be refolded back into header. the main_menu region I added could also likely be removed, but for the sake of clarity might be best if kept. All of these issues are fairly irrelevant to this specific issue but matter to the whole discussion. Also, when using the new block I added here, it becomes pretty obvious that we need to be pushing block title up to administration which might solve a few things elsewhere but is outside the scope of this issue.
Eclipse
Comment #19
sunI think that's the overall direction we want and need to go. :)
That said, I think it's too limited/focused. I think we should have marked this as a duplicate of #503810: Convert primary and secondary menus to blocks instead. We're repeating the entire discussion. :-/
In essence, the conclusion in the other issue(s) was long before that we need to get rid of all of this, entirely. Instead, we have a single MenuBlock plugin type, which comes with (almost all of) the settings that the contrib Menu Block module provides.
Reality is, that's what everyone actually needs. The entire progress on #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables was only halted, because it would have had to introduce a stop-gap block-instance-alike API just for menu blocks, and I didn't want to waste any time on duplicating that effort with a temporary API that gets deleted later on.
Therefore, if we really want to do ourselves a favor, then we skip all of the intermediate steps in between and directly hop onto the actual, final need of site builders and users, which means:
@see menu_block.admin.inc
I'm fairly sure that we should be able to copy/paste a lot of menu_block's code. There have also been a range of menu link tree building functions in the meantime (most notably,
menu_build_tree()
, which allows for custom, parametrized link tree builds) that should make all of those configuration parameters a piece of cake to implement.In summary: Instead of introducing a MenuNavigation block plugin type, in addition to the already existing menu blocks in HEAD, we should get rid of them altogether, and replace them with a single MenuBlock plugin type that all of us actually need and want. :)
I'm happy to help. :) And I hope that @JohnAlbin is available, too. ;)
Comment #21
EclipseGc CreditAttribution: EclipseGc commentedSo, the MenuNavigation block I just wrote in that patch does everything you asked for except for min/max depth. It just expects a single level. I think we might have to render it differently if we do the variable depth thing, but I understand why you want it. Otherwise, there's not really a need for the "follow" feature as the methodology for doing that doesn't actually have to know about another block on the page. If it can display items of the 2nd level for a menu (because the current page is part of that menu and has children or whatever) then it will.
I'm open to what you're discussing, however I think the patch I just provided is a pretty good one for one replacement of what's in core, it's small, and won't creep scope wise. If we fixed the tests that are breaking here and added a few new ones, it could probably be committed in the relatively near future, and we'd ALMOST have menu_block in core as well. Expanding on what that plugin does and removing the other plugins that provided similar functionality would be a good follow up IMO.
Eclipse
Comment #22
sunHm. This should pass some more tests, or at least reduce the amount of exceptions...
But there's a critical problem:
Thousands of tests expect at least the "Account links" (account) menu to appear as secondary links, by default, in all themes, including Stark.
With this patch, that's no longer case, since 1) Block module is optional, and of course, 2) the block instance config to place the account menu doesn't exist.
Speaking, very unhelpfully, from a Testing system perspective, less baked-in assumptions and defaults are great ;) However, that doesn't really help ;)
FWIW, I pushed this as menu-links-1869476-sun branch into @EclipseGc's Blocks/Layout sandbox.
Comment #23
Gábor Hojtsy@sun: I think that is fine as long as enabling and placing the block is tested too. :)
Comment #25
sun@Gábor: I'm not sure my point came across. ;)
The critical problem is that, as of now,
$this->assertText(t('My account'));
).Removing this dependency from tests is a mammoth undertaking, similar to the size of #1541298: Remove Node module dependency from Testing profile.
Adding Block module as required module (via .info), or adding Block module to the Testing profile, and providing default configuration via Block module or Testing profile, would be a stellar, ugly idea. ;)
Postponing on #1894692: Allow to identify whether a user is logged without presence of Account menu links as secondary links
Comment #26
EclipseGc CreditAttribution: EclipseGc commented@sun
ugh, that was a much clearer communication, and I didn't like anything you said (not your fault, just sucks that the tests are dependent on that). Understanding that this is postponed based upon your very relevant points here, is the basic functionality still an ok target for a first pass in your opinion?
Eclipse
Comment #27
sunYes, I think it's definitely a step forward. I'd agree with trying this first, but my secret hope still is that, once this patch is green, that it should be a no-brainer to merge the new MenuNavigationBlock class into the existing MenuBlock (since those two would really be strange duplicates to each other), which in turn would bring us to ~50% of menu_block in core already. The other configuration settings can be added in a follow-up issue. Of course, the merging could also happen in a separate follow-up issue, but I'm allowed to have my secret hopes, right? :)
Comment #28
dcrocks CreditAttribution: dcrocks commentedWill the tests still fail if Bartik is modified to use 'Main Menu' and 'User Account Menu' blocks instead of primary/secondary links? It would remove a lot of code from bartik. Is this feasible as a separate issue?
Comment #29
sun@dcrocks: Yeah, they will still fail. The issue is not Bartik, nor the Standard profile. Instead, the issue is with any test that is not using the two, which means >90% of all tests. ;) Meanwhile though, I've provided a patch for #1894692: Allow to identify whether a user is logged without presence of Account menu links as secondary links. With that in place, the total amount of failing tests should significantly decrease here.
Comment #30
dcrocks CreditAttribution: dcrocks commentedSo the focus is still on removing them altogether. My interest is just in removing Bartik's use of primary/secondary links. Thanx for the insight.
Comment #31
sun#22: menu.links_.22.patch queued for re-testing.
Comment #33
sunHrm. Initial attempt to make the OpenID tests work, but this is non-trivial to resolve. Already spent a good chunk of time on those, still looking into it.
Comment #35
sunTo get a better grasp of the new block plugins, I skipped over the broken OpenID tests for now, and instead, moved on to the UserAccountLinksTest, which apparently is a dedicated test class for the former secondary links functionality.
I made sure to document the melting of my brain in #1871696-123: Convert block instances to configuration entities to resolve architectural issues and following comments, in case anyone is interested. :)
Now, with that being grilled, this Hawaii Toast should at least show a green light for UserAccountLinksTest. ;)
Comment #36
tim.plunkettOverall, I really like the direction this is going. Especially the changes to the template files :)
Missing docblocks
You can just use $this->configuration here
Missing @param
All of this block stuff looks good to me
Implements
These should be typehinted, array and Drupal\block\Plugin\Core\Entity\Block respectively (with a use statement)
$block->id()
Is this also for mimicking existing functionality? This makes me really uncomfortable, I could picture someone angrily filling in the block label over and over wondering why it doesn't take effect. Maybe we should form_alter out the label for these?
Eww. Is this temporary? Just to mimic the current markup? If so, let's add a comment about that. And can prefix/suffix be moved to each other
Comment #38
EclipseGc CreditAttribution: EclipseGc commentedI think the appropriate thing to do here is to provide a different theme wrapper, so yes, I'd consider them temporary (and the wrapper probably in a followup).
Eclipse
Comment #39
sunHah, OK, nice, that explains the increase in the test failures:
I guess I'll simply slap a
drupal_html_id()
onto the #attributes][id of the block in the next reroll, but I'm very certain that I actually opened a can of worms there :-DComment #40
mgiffordAfter applying the latest patch, should I be able to see the primary links & secondary links blocks here:
admin/structure/block/list/block_plugin_ui%3Abartik/add
If so I must be doing something wrong.
Comment #41
dcrocks CreditAttribution: dcrocks commentedIt is there but as one block only. It is called 'Menu Navigation'. In this block you can configure the target region, source menu, and the number of levels to display. In the 'Main Navigation' and 'User Account menu' blocks you can only configure the target region. It appears the 'special' behaviors of primary/secondary' links are preserved. Perhaps there should be a better name, as this one doesn't stand out from 'Main Navigation'. The other 2 blocks are named after the menus they display. It seems this one should contain the word 'Menu', as it does dish up menus, and I know there is a concern about including the word 'links', perhaps just 'Menu Display/Print/??
It is interesting that on the Menu module settings page there is no longer any defaults set. Does this form fit any purpose anymore?
Comment #42
dcrocks CreditAttribution: dcrocks commentedActually I would vote for 'Display a Menu', since this block can display any menu available to the site.
Comment #43
dcrocks CreditAttribution: dcrocks commentedCould the new regions added to Bartik have more generic names since I can punch whatever menus I like into those blocks? Maybe 'Top of Page Menu Bar' and 'Top of Content Menu Bar' or something similar?
Comment #44
dcrocks CreditAttribution: dcrocks commentedThis may not belong here but I notices this as I was playing with this. If you have a block currently assigned to a region but then go into Add Block and assign the same block to a new region, without changing its 'Title', it simply gets moved from the old region to the new one. Reasonable I guess, but if the intent was to ADD a block, not just MOVE it, then it may be a somewhat surprising result. I also noticed that if a block was once assigned to a region but is no longer assigned to any region, its config .yml file stile exists in sites/default/../active directory.
Comment #45
Gábor Hojtsy@dcrocks: re #44 those are unrelated pre-existing bugs or just current behaviours with the blocks system.
@sun: I agree with tim.plunkett that this looks really good. Great direction :)
Comment #46
dcrocks CreditAttribution: dcrocks commentedWould it be conceptually simpler to take the block you used for 'menu-navigation' and use it to build all the menus in drupal? Is there any feature of any of the core 4 menus that couldn't be built with this block? It would seem to add flexibility and, I think, be simpler conceptually to drupal developers.
I'm not sure I am being clear. I am trying to say that all the core menus would be assigned to a region through this block. Now, every menu, the core 4 or any new site menus, show up in the 'add blocks' table as unique blocks. Adopting this would mean only one block, menu-navigation, would show up in the 'add blocks' table, and it could be used to place any menu in any region. This can be done with the patch as it currently exists so it seems wasteful and confusing to have these other menu blocks also in the list. When done, what is the difference between using the 'add blocks' form to place the 'tools' block in a region and a 'menu-navigation' block that selects the tool menu?
Besides making the 'add blocks' list shorter and simpler, it would also mean that creating a menu wouldn't also create a block.
Comment #47
EclipseGc CreditAttribution: EclipseGc commentedThat's outside the scope of this issue, but likely the answer is yes. The bigger problem here is that you want a way to display them through different theme functions if you're going to do that, and getting a list of theme functions that support rendering menus isn't something drupal supports right now (and is something we could REALLY use and for more than just menus).
Eclipse
Comment #48
sunHm. I think I disagree there. The Navigation menu (btw, it's called Tools now) on its own just represents the data source, a menu/links tree.
1) If you create a block that uses this data source, then you should be able to configure how the data source is handled and rendered for the particular block instance you have.
2) You can have multiple block instances for the same data source.
3) The underlying idea of #46 would rather translate into "a block instance of a block instance". That's a configuration and dependency territory that I don't think we want to get in.
4) Instead, it's much simpler to just be able to create multiple block instances for the same data source, but configurable them differently.
5) The actual output is (or should be) always the same: A HTML bullet item list (UL/LI). This is universally true. The only thing that makes any difference is the CSS class that is applied to the wrapping container: That's what gets you from "a list of links" to menu link trees to local tasks to local actions to contextual links to dropbutton links to entity action links to pagers to... etc.pp. — In turn, we're circling back into many related discussions: We need a Theme Component Library, and, as already mentioned here, elsewhere, and in #503810: Convert primary and secondary menus to blocks, we essentially need a way to "classify" blocks, so a theme is able to apply discrete styles for particular block, whereas the raw HTML markup does NOT change, only the CSS class of the block does.
Comment #49
dcrocks CreditAttribution: dcrocks commentedI think what this is is a block instance of a data source of a particular type. If the HTML gave instance specific information as well as data source type I would probably have all I need for my CSS. Ie, instance is menu block called X in region A of data source type 'tool menu' I can write specific CSS as well as use generic menu type CSS. In any case if this patch goes through as it stands I do have the generic menu assignment block I want, plus a few data source specific blocks I will probably ignore.
Comment #50
mgiffordThanks for the clarification @dcrocks, I found it.
Should there be a checkbox in the UI to hide the block title using element-invisible? I think that's going to be a common request and users aren't going to want to dig through the CSS to remove them that way. Would be nice if it were just a simple option when adding/editing a block.
Comment #51
sun@mgifford: #1875260: Make the block title required and allow it to be hidden
Comment #52
webchickTagging.
Comment #53
mgiffordThis patch #1079010: The secondary links heading, "Secondary menu", is wrong is marked RTBC and was ready to go, but is waiting on this issue here as it would obviously over-ride it.
I haven't seen a lot of movement on this in the last week though, so what should I do about the patch that was ready to go?
Comment #54
sunPlease note:
The bartik_block_view_alter() override generally looks bogus/needless to me. However, we're currently lacking a proper way for block plugins to control and adjust the 1) wrapping block markup (e.g., via
buildWrapper()
), as well as 2) .element-invisible for block instance subjects. These two details are discussed in at least two other issues already.Comment #56
mgifford#54: menu.links_.54.patch queued for re-testing.
Comment #58
sunComment #59
sunWith #58, hopefully only the upgrade path test failure will remain. And, that one actually gets very very tricky:
As a result, the upgrade path would have to look like this:
The only alternative to that would be to skip the upgrade path and require all sites to (re)configure/place their primary/secondary links blocks. After upgrading, you'd essentially end up with a UI/output that is similar to the Testing profile used by tests: The lack of a main menu/primary links is probably not a big deal — but the lack of the account menu/secondary links very potentially is :-) One possible way to work around the situation would be to change the final link in update.php (that gets you back to the updated site) to point to the /user path instead of the front page. That way, you at least find your way to the login page ;)
Comment #61
sunOh lovely to see that block entities/plugins are completely ignoring default block plugin settings...
That inherently means that our test coverage for blocks is very incomplete and fragile — if default settings are not merged, then settings may be undefined, so either all runtime code is currently using conditional fallback values, or lots of tests will blow up as soon as you merge in default settings in the block manager.
Comment #62
effulgentsia CreditAttribution: effulgentsia commentedWe've been generally categorizing conversion issues as tasks, not features. If there's a reason this needs to be considered a feature, please explain.
Comment #64
andypost@sun please re-roll #61 and what's left here to finish conversion?
Comment #65
mgiffordBump. I was just checking into #1079010: The secondary links heading, "Secondary menu", is wrong and realized that this issue's still hanging.
Comment #66
sdboyer CreditAttribution: sdboyer commentedjust noting that this is a blocker for SCOTCH, and the work i'm currently doing in our princess branch.
Comment #67
webchickI'd love a standalone patch that did this, if it's easy to separate. This has value even outside of SCOTCH.
Comment #68
webchickThis isn't in Spark's wheelhouse from what I can tell. Untagging.
Comment #69
Bojhan CreditAttribution: Bojhan commented@webchick I thought Spark wanted to put contextual links everywhere? Isn't this part of that effort.
Comment #70
webchickMaybe eventually? But not really right now. We're trying to get our own features/problems done, rather than other peoples' atm. ;) I think it makes more sense as part of the "blocks everywhere" initiative since it's, well, putting blocks everywhere. :D
Comment #71
EclipseGc CreditAttribution: EclipseGc commentedjust for reference I completely agree with #70
Comment #72
larowlanRe-rolled against head
Comment #74
larowlanCleanup for changes to annotation namespace
Comment #77
larowlanComment #79
larowlanShould be green, passes locally
Comment #80
larowlanScreenshot of bartik
Comment #81
larowlanFixes two comments longer than 80 chars identified by @swentel
Comment #82
sdboyer CreditAttribution: sdboyer commentedok, i've been through this. i'd feel better having someone who's actually GOOD at knowing all the coding standards, etc., RTBC it, but technically, it's good, so i'm gonna do it anyway. the only issue with it is the two new regions it introduces into Bartik to accommodate the new blocks, since they're really intended to just be single-purpose regions.
fortunately, we have a better solution for things like this in princess :)
Comment #83
tim.plunkettThere's a bit more here than coding standards issues. The changes to test coverage are especially worrisome.
I don't think we should kill this generic function and relegate it to a protected method of a block plugin.
Yes, we don't want to call out to a procedural function, but I think we should for now and move that code later.
Is this needed? If so, needs a comment
Unneeded
None of this should be changed, at all. This sort of change should definitely not directly impact Open ID
Out of scope
Needs a typehint, and @return
protected
Is $block a documented object property already? If not, it needs to be
Missing a docblock:
Don't we usually start with 8001?
Uuid, unfortunately
Comment #84
David_Rothstein CreditAttribution: David_Rothstein commentedGotta love seeing contextual links on these things :)
8000 is correct, but more to the point, why is there an update function in the Standard install profile? I do not understand why there would be an update function that only sites running this profile would need.
Plus the update function seems to be assuming the site is using Bartik for some reason...
Why is this region called "account links" rather than something like "secondary links" or "secondary menu"? There is no reason account-related links need to be placed there.
I would think it should start at "1", not "0"...
I am not sure what this means, but "respecting #access" sounds a lot more important than a @todo :)
Comment #85
larowlanThis changes those things identified by @timplunkett.
Pretty sure we'll get 68/17 fails/exceptions on open id, even placing the block. No idea why this changes open id.
Comment #86
dcrocks CreditAttribution: dcrocks commentedEven though the default configuration for bartik is to have the 'user account' menu as the default source for the secondary links it does seem confusing to have the region and block label to be called 'Account links' when the secondary menu concept continues to exist in the rest of the code and other menus could be substituted with actual use.
And though menu.settings.yml is virtually empty now it is still used within drupal. The form should be modified temporarily but not dropped as I expect there will be future use.
Comment #87
larowlanstandard.profile installs bartik as the theme. It needs to take care of updating bartik (unless bartik can take care of itself - I don't think it can). It doesn't know about any other themes the user has installed since.
We cannot know what a contrib theme will call this region. The upgrade path for that theme will need to handle placing the block in the correct/relevant region. In most cases this will be a new region for that theme. So I guess we need a follow up to a) allow themes to implement update hooks (assuming thats not already possible) and b) remove the standard_update_8000() in favor of bartik_update_8000().
The attached patch removes that page, lets see what the tests say. The functions that use those settings are gone too, I think thats why the tests passed. There was only a test for a 200 response on the admin page but nothing else (that I can see). (i.e. we did not have test coverage that I can see) - but lets see what the bot says.
menu_navigation_links adds 1 to the passed level, we need to start at 0.
Fixed
Fixed
Comment #88
dcrocks CreditAttribution: dcrocks commentedCouldn't we just 'empty' the menu.settings form for now and leave an issue to clean it up later before 8 goes final? It's quite possible a contrib module uses the settings form and I plan on using it as well. It seems a waste to remove it then have to restore it. I don't see menu.settings.yml itself going away.
Comment #89
dcrocks CreditAttribution: dcrocks commentedA couple of suggestions. Eventually, everything in the bartik header will be blocks(ref: #1053648: Convert site elements (site name, slogan, site logo) into blocks). Couldn't the secondary links be put in the header region in bartik with enough weight that its block would always be first and then add css to style? Save introducing a new region to d8 that would have to be explained. Also, since main_menu might not always have 'Main menu' in it, why not use a more generic region name in bartik, e.g. 'menu_bar', that is in wide use and is conceptually what this is?
I know these are just cosmetic but it might make things easier down the road.
Comment #90
tim.plunkettI'm not going to RTBC this, because I'm still uncertain of the standard_update_8000() parts.
menu_parent_options() indicates that override_parent_selector is on the chopping block anyway, so I'm not worried about deleting that form altogether.
I'm equally unconcerned about the naming of these regions now, as we add more of these, we can revisit the naming.
The tests look good.
Comment #91
dcrocks CreditAttribution: dcrocks commentedApplied cleanly on a fresh install of 8.x. Still think block names main_navigation and menu_navigation are going to cause confusion and wtf's. Sad about the settings form. This issue needs to worry about #1961870: Convert page.tpl.php to Twig and #1938840: bartik.theme - Convert PHPTemplate templates to Twig because whatever lands 1st will cause changes in the others.
Comment #92
David_Rothstein CreditAttribution: David_Rothstein commentedThose changes mostly looked good to me.
I still don't see the point of an update function that runs for one combination of install profile and theme only, though... Also, it doesn't seem to be respecting the site's existing settings - just putting two default menus in there?
I would think it could be something more like this (pseudo-code, and some of these functions probably aren't safe to use during updates):
This will not get themes that choose not to support the new default regions, but there's not much we can do about that, at least not now.
Comment #93
David_Rothstein CreditAttribution: David_Rothstein commentedActually, @sun in #59 already outlined something like this, but with some details I was missing (update function needs to be in System module, etc). To keep things simpler perhaps instead of enabling Menu and Block modules if they're not on, the update function could just skip running anything if they're not already on.
I think some sites probably will need to do manual steps here to get things the way they want.
Comment #94
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think it's a problem if we need to do some simple math behind the scenes before passing data into that function :) Humans looking at the user interface, however, should really see numbers that start counting at 1, not 0....
Comment #95
larowlanYeah that is a better idea, match the configured primary/secondary menus instead of hard-coding account/main-menu.
Will sort the 0/1 issue - agree that is a better UI.
Thanks
Comment #96
larowlanWhere do we sit with this one in regards to risk-release?
Still worthy of a re-roll or is this lumped with Scotch?
I think it stands on its own.
Comment #97
Gábor HojtsyI think this stands alone, not dependent on scotch at all.
Comment #98
CBOk, rerolling origin/8.x into this.
Had a few conflicts, merging was pretty straight forward.
Still a warning that I haven't been able to fix;
Comment #99
andyceo CreditAttribution: andyceo commentedComment #100
larowlanChanges to page.html.twig
Comment #101
tim.plunkettThis removes action links and tabs, but doesn't seem to add it anywhere else. Shouldn't those be left for http://drupal.org/node/507488?
Comment #103
hosef CreditAttribution: hosef commentedOk, here is a new patch in which I have re-added the items that @tim.plunkett mentioned, removed a preprocess hook that is no longer being used, and modified the CSS so that everything looks the same as it did before.
There is still an issue with the block definitions that needs to be fixed.
Comment #105
hosef CreditAttribution: hosef commented#103: menu-blocks-1869476.103.patch queued for re-testing.
Comment #107
CB#103: menu-blocks-1869476.103.patch queued for re-testing.
Comment #109
oadaeh CreditAttribution: oadaeh commentedRemoving the unnecessary B&L tag.
Comment #110
hosef CreditAttribution: hosef commentedreroll
Comment #112
EclipseGc CreditAttribution: EclipseGc commentedNot an endorsement, just rerolling after the removal of openid stuff.
Eclipse
Comment #113
EclipseGc CreditAttribution: EclipseGc commentedvacation brain...
Comment #115
dcrocks CreditAttribution: dcrocks commented#112: menu-blocks-1869476.112.patch queued for re-testing.
Comment #117
klonos...please don't shoot me, but I think @sun hasn't worked on this since back in March. Unassigning issues when not actually working on them helps get more eyes I think.
Comment #118
jibranNeeds reroll as per #116.
Comment #118.0
jibranUpdated issue summary.
Comment #119
swentel CreditAttribution: swentel commentedThis would still be cool to get in and then further explore 'menu block' options in #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables.
Rerolling - let's see if I got this right.
Comment #121
swentel CreditAttribution: swentel commentedThis at least brings the links back (but they're ugly) - uncommented the upgrade path for now, it feels weird indeed being in standard.install.
Comment #123
swentel CreditAttribution: swentel commentedFixed the exceptions - couldn't get the right theming though
Comment #124.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #125
klonosComment #126
RainbowArrayWorked on this related issue last night: [#/1053648].
Hoping the work there will help to move this issue forward. I read through the related issues, so I'll take a go at this in the next couple days.
Comment #127
Jeff Burnz CreditAttribution: Jeff Burnz commentedWouldn't it make a more sense to do markup changes in a template and not in hook_block_view_alter?
The regions are very specifically name, to me this is like a sidebar-user-login type region, which we would never do - the header is effectively going to end up with 4 regions, perhaps instead something like header.top header.bottom etc, personally I don't care much about the convention used but using very specific naming based on a single use case seems out of place.
Honestly I am still rather bewildered why we need this and don't instead heavily pursue #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables, is it too late for that now?
Comment #128
RainbowArrayMy understanding is that by using the approach in #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) for this issue, we'll essentially end up with the equivalent of menu_block in core. I was going to wait until that other issue was committed before starting in on this one. I do think we'll probably want to take a similar approach where the first step is getting the ability to add a menu block, while the second step is altering the regions to replace the markup in the templates with an installed menu block.
Comment #129
sdboyer CreditAttribution: sdboyer commentedthis approach is broken. we're conflating way too much into regions, per #127. while the patch @mdrummond links to in #128 is a good-enough solution for Bartik, it's not actually changing the way regions *work*. they're still generic containers, and the basic concept behind them needs to be enriched for the abstraction to still work well. designing for Bartik doesn't help us.
Comment #130
Jeff Burnz CreditAttribution: Jeff Burnz commented@sdboyer not sure or clear on what you mean by:
Comment #131
andypost"branding issue" is in! Suppose the primary reason for the issue is to decouple menu from theme layer.
Actually menu & menu_link modules could be disabled but theme is hard-wired.
The menu block is nice idea to get rid of
SystemMenuBlock
that now does not belongs to system but it's separate issue.variables could be empty
theme settings?
But form still here and config schema too!
Why menu? it's optional module, and as block render links this should be moved to menu_link module
and others... let's implement helper method for.
needs to be addressed
config now shipped without uuid
no more needed
Comment #132
RainbowArrayHere's a completely reworked version of this patch based on the approach used in #1053648: Convert site elements (site name, slogan, site logo) into blocks.
In short, all this patch does is create the ability to place a block with any particular level for a particular menu. That's it. We can do a separate issue to create the regions needed to replace the primary menu and the secondary menu and to actually place menus by default.
Based on the discussion above, it seems like it would make sense to get this basic level in first, and then if we're still able to add some of the other configuration options that the Menu Block modules, we can do that in a separate issue.
I renamed this MenuLevelBlock, since that's what this does, and since there was some concern above about blocks named menu_navigation when there are main_navigation blocks too. We can easily change the block name if necessary or place this in menu_links. It should be noted as far as I can tell, menu_links is only required right now because of some work being done on it, at least according to a @todo that I found.
I tested this out, and it works well. It would do the job of secondary menus, in that if you specify level 1, for example, it will only show that level if you are on a page that has child items in level 1.
Hopefully this will help us get this done.
Comment #133
andypostIn case the new block introduced here then current
SystemMenuBlock
with derivative should be removed in favor of new one. Having both is confusing.Also this new block is not covered with tests
EDIT there's a special issue for this block #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables so this one should focus on theme integration and probably wait for menu.inc removal
Comment #134
tstoecklerhehe, de ja vú?
Comment #135
RainbowArray@andypost: Yes, there's definitely duplication of discussion in #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables, although all the work on actual patches has been done in this issue, rather than that one, which is why I posted this here.
I'm aiming to use the same process as we used for #1053648: Convert site elements (site name, slogan, site logo) into blocks, which was to first get in a patch that allows for the actual creation of the block, and then have a follow-up issue which deals with removing the variables from the page templates and replacing them with a pre-installed block. Breaking those two things up seemed to make it more manageable so we could actually get things in. If we use #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables for the first part, which allows for adding a block, and this issue for removing the page variables and replacing them with the appropriate menu blocks, I'm fine with that. Whatever will get the job done.
I took a quick look at SystemMenuBlock, and it appears to be what allows for custom menus to have a block, which is somewhat different than this, which allows for a particular level of any menu block.
@tstoeckler: Oops. I used SystemBrandingBlock as a template for this revised block: missed changing that bit. I'll fix it in the next patch.
Comment #136
RainbowArray@andypost: After chatting with EclipseGc on IRC, we decided it would make sense to make the level selection tool available as an option for the existing menu blocks, which would mean adding this to SystemMenuBlock as far as I can tell, rather than having a separate MenuLevelBlock class. I think that's what you were saying, so my apologies if I misunderstood.
Comment #137
lokapujyaShould we remove: "Introduce regions as needed to place these elements." from the issue description?
Comment #138
RainbowArrayAfter some further discussions, I'm going to move the work on patches to #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables, as this first step is much more about getting the essentials of menu_block into core. Once that's done we can come back here to replace the primary_links and secondary_links variables with blocks.
Comment #139
dcrocks CreditAttribution: dcrocks commented+1 Looking forward to it.
Comment #140
Wim LeersIOW: those two blocks are now responsible for 27 DB queries. As soon as they are blocks, ~10% of DB queries will be removed, because
SystemMenuBlock
are render cached by default.Comment #141
catchSomething seems broken with that number of database queries. Drupal 7 can just about serve an entire page with 27 database queries.
I'd be tempted to open a new issue for the regression, and have this as a sub-issue, but leaving this as is for now since we may well need to fix both the cached and uncached state separately anyway.
Comment #142
catchComment #143
catch#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees should fix the performance issue here, so moving back to major.
This would still be good to have in its own right, so leaving as beta target. We could add the capability at any time, but can't remove the template variables post release candidate.
Comment #144
RainbowArrayJust a note that I have a working patch up on #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables. Trying to get some reviews so we can get that in.
Comment #145
Wim Leers#474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables is now RTBC. Now let's get this one moving forward again! :)
Note: This is a soft blocker for #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, because it would make that patch a lot simpler: none of the hacky, one-off rendering of primary and secondary links in the theme layer and in
menu.inc
would need to be updated by #1805054 anymore, making that patch significantly simpler.Comment #146
Wim LeersHurray, #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables now got committed for realz!
Let's get this one moving forward again? :) I'd be happy to provide very frequent reviews!
Comment #147
Wim LeersSo, here's a kickstart for this issue. This patch:
menu.inc
,theme.inc
andcommon.inc
admin/structure/menu/settings
, since that's obsoleteStill left to do: make this match the current (HEAD) visually, i.e.:
(See the
@todo
s.)Let's see how spectacularly this test will fail… :D
Comment #149
mgiffordFailed pretty good. :)
I tried locally & couldn't apply it either.
Comment #150
thinkyhead CreditAttribution: thinkyhead commentedOn track for the next big D8 core sprint — Sept 27, 2014 in Amsterdam! Wish I could be there. Subscribing to this issue in the meantime.
Comment #151
Wim LeersStraight reroll of #147.
Comment #153
Wim LeersAs promised, I'm pushing this forward. I will definitely need your help though, mdrummond, to finish this patch.
The first thing I wanted to do is to get the main menu to look and behave the same. I got that working, including markup I didn't know existed that is used to "toggle" the main menu while responsive in Bartik, that was added in #1880488: Make menu collapsible on small screen resolutions.
Next, I tackled the secondary menu. The question is how we distinguish between the main and secondary menus. We don't need to care about a third menu in the header, because Bartik in HEAD also doesn't support that (well, it renders, but it's so ugly you'll run away screaming). Therefore I went with the simplest thing I could think of to determine which menu gets which styling: the first menu block in the header region gets the "main menu" styling, including the toggle. The second menu block in the header region gets the "secondary menu" styling. No work has been done yet to make it work for additional menus.
The above explains why the CSS selectors have gotten so much bigger: rather than having known IDs to target (
#main-menu
,#main-menu-links
…), we now have to target the first/second menu block in the header, so that becomes#header .block-menu:first-child
and#header .block-menu:nth-child(2)
.Of course, Bartik was written with the assumption of primary and secondary menus being part of the theme, so the CSS still reflects that. Making the CSS cleaner, more generic and above all simpler is something that can be done in a follow-up, I believe.
Finally, this patch also fixes all test failures. Most of them were due to assuming the "account" menu was the secondary menu, and that it was available from the theme. That assumption no longer holds, but is easily fixed by just pldacing a block that contains the "account" menu.
Comment #154
Gábor HojtsyAmazing. Looks like needs frontend reviews first and foremost :)
Comment #155
rteijeiro CreditAttribution: rteijeiro commentedA few minor nitpicks.
Also started to do some manual testing.
Comment #156
Gábor HojtsyGood trick to tie the CSS to the position of the block in the region IMHO, although not sure if this will be evident to people. BTW as-is visually looks good except the red border:
Don't think this red border was intended?
Also wondered what happens when they place a third menu? It sort of falls apart :/
Comment #157
Gábor HojtsyBTW I also noticed no "Edit menu" contextual link on the block, but that is not on the tools block or the footer menu block either post/pre patch. Duh. How do we not have a test for that... One of the great side effects of this patch will be you can edit items right from a contextual link on the primary/secondary menus. Quickest menu customization ever. If it works :) Should be fixed in a separate issue as it is pre-existing.
Comment #158
Wim LeersLOL, that red border was just there for debugging purposes, I forgot to remove it :P
I'm hoping the smart frontend folks here can come up with a solution when >2 menus are used. But note that also in HEAD, things break down in that case, even though it breaks less in HEAD.
Comment #159
Gábor HojtsyIts right, we don't necessarily need to resolve the 3rd menu looking bad because in head already the first one will look bad :D Confirmed.
Comment #162
LewisNymanIs this a really good idea? This seems really fragile. Our CSS standards intend to prevent this kind of fragility.
Comment #163
Wim Leers#162: I don't know at all if this is a good idea. My goal was to get the patch to a point where it works. I'm hoping you and others with much stronger CSS & Twig skills than I have can push it to the finish line. If you don't have the bandwidth or interest to do that, I'm hoping you can provide guidance/reviews so I can keep pushing it forward.
:)
Comment #164
RainbowArrayI'll try to take a look tonight. Thanks for your work on this Wim!
Comment #165
Wim LeersIn this reroll:
system_theme()
didn't set'render element' => 'elements'
. That means contextual links on the system branding block is currently broken in HEAD also.)#164: Thanks!
Comment #166
lauriiiI think at least some of the CSS fixes might need this to be done before: #2318611: Menu block template doesn't provide the menu name at all
Comment #167
lauriiiI did some testing and seems like the blocks are getting attached on installation but now menu is in the header region which is wrapped with
header
. Is that ok for us or should we create new region for the menu?Comment #168
dcrocks CreditAttribution: dcrocks commentedAll the attempts at this in the past created new regions for both menus, usually both in the header element with the header region.
Comment #169
lauriiiComment #170
lauriiiFixed some more CSS. Hopefully @rteijeiro gets his dreditor working to keep trolling going on.
Comment #172
LewisNymanThis is much better! One change that would be more inline with our standards is .menu-main -> .menu--main and .menu-account -> .menu--account
There are a few situations where we can make these selectors shorter and weaker but removing unnecessary .menu's, li's, etc in the selector.
Comment #174
lauriiiChanged the class name to one that Lewis suggested and fixed block names on tests.
Comment #178
lauriiiComment #179
lauriiiComment #180
Wim LeersActually, unless I'm mistaken, this is very wrong :(
This ties this styling to a specific menu: the "main" menu. That menu just happens to be created by default as part of the Standard install profile. What if you want to use a different menu as the main menu? What happens if you don't have a menu called
main
?This is precisely why I went with the order-based approach. This represents a huge regression compared to HEAD…
Comment #181
LewisNyman@WimLeers Ok, if we want to mimic the current styling in HEAD then we just convert the
#main-menu-links
into a class, and add that in a template override. We can still called it.menu--main
as long as it isn't being automatically generated from the machine name right?Comment #183
Wim LeersBut how are you going to determine when to add that class?
Comment #184
joelpittet@Wim Leers that seems to be the responsibility of the block to add that class, no?
Which in d7 could be exposed via something like https://www.drupal.org/project/block_class
Likely in the template I'll be doing something along the lines of
<nav class="menu--primary">{{ navigation_primary_region }}</nav>
And dump 1, 2 or more menu blocks into that region.
Followed by a few more regions for posterity:)
Not totally sure yet if this patch accounts for all the use-cases but by the @todo, I think not quite yet.
Thanks for pushing this issue along though, I intend to experiment with it next weekend!
Comment #185
xjmHere are the API and config schema changes I saw scanning the patch. I added API and data model changes to the summary -- please check that they are complete/correct.
This change will need a change record. There is also a BC break in the public menu API, so we need to consider doing this by beta 1, preserving some form of BC, or whether this is an okay BC break in a beta.
Comment #186
xjmTagging as a priority for a pre-AMS beta sprint.
Comment #187
RainbowArrayI looked this over last night and noodled on it overnight, and I think we may want to approach this a different way.
The concept of the primary menu and the secondary menu is that you could select any menu for each, and it would appear in a specific place within the theme and behave a certain way.
Because we're switching to using blocks, I think we need to change the way a site builder can make those same choices. The sanest way to do so is to have a specific region for the primary menu and a specific region for the secondary menu.
We can certainly have default menu blocks that go into those regions, but this approach would make it pretty simple for somebody to easily place a different menu block in the primary menu region and another in the secondary menu region.
This is also a nice way to handle things because each theme can do this however they please. If they want a primary menu region, great. Don't want a secondary menu region? No problem. Want a tertiary menu region? Sure. If a themer wants to set up a certain set of styles for menus that appear in a particular region, they can do so.
Once challenge worth reviewing is what happens if somebody places multiple menu blocks in a menu region. I think the answer to that though is that if it looks weird when somebody does that, a site builder will be able to see that easily and avoid doing that if it causes problems.
I'll try to find some time to work on a region-based approach to primary and secondary menus. If you have concerns with that approach, feel free to shout about it, though!
Comment #188
Wim Leers#184:
No, because that implies we'd need to expose a checkbox for menu blocks that essentially says "render this as the main menu". That's very problematic. But, fortunately mdrummond provided comprehensive thinking around this in #187, so let's continue the discussion using #187 as a frame of reference.
#187:
And this is why I went with the approach I went with. It allows you to make the same choices, without the problems associated with introducing special-purpose regions.
… this is all true, but …
…Voila! That's the problem with special-purpose regions! And it's not limited to that: nothing prevents the user from putting blocks other than menu blocks in this special purpose region.
For this to work well, without problems, we'd need the concept of a region that only accepts certain types of blocks. But that's also problematic (what if you have a module that provides an alternative, more customizable menu block? Then those blocks could not be placed in this special purpose region, which is also a WTF).
I hope the concerns above are clear to you.
In conclusion…
The thing is: I don't see a way to do this without something being problematic:
header
region. It seems that's problematic because 1) ugly & less efficient CSS, 2) not so great discoverability/understandability..main-menu-region .block { display: none; } .main-menu-region .block:first-child { display: block; }
, which is … very dubious and will also have a terrible SX (Sitebuilder Experience).So the SX is objectively bad in all four cases:
Options 1 is clearly fundamentally broken. Option 3 is acceptable for a site-specific theme, because there it is known which menu will be the "main" menu. Options 2 and 4 are both viable, because they're both solutions for themes that aren't site-specific: they allow the user to still choose the "main" menu.
I don't see a 5th or 6th option. If anybody sees another way of doing this, please let us know!
So that leaves option 2 and 4.
Perhaps we can get away with option 4 and a simple solution to its problem after all: attach some JavaScript to
admin/structure/block/%theme
that shows a dialog to the user whenever a second block is added to this special purpose region, and that ensures the placed block is implemented by theSystemMenuBlock
plugin?Comment #189
RainbowArrayI think you summed up the issue very well, Wim. There isn't a perfect solution.
Adding some sort of warning with option 4, seems decent. I think this gets partially solved by the fact that people will see pretty clearly what happens if they put in more than one menu block, if it looks weird. Also the fact that the regions have menu in the name makes it clear you shouldn't put non-menu things in there.
People can do all sorts of silly things, and we can't prevent all of them. Having a somewhat sensible what way for people to make a good choice is what should be our aim.
I got started on a patch, but headed off for the afternoon/evening. I will try to wrap up the patch tonight and get it posted then. Might be easier to evaluate options once it is is easy to see how this would work.
Comment #190
dcrocks CreditAttribution: dcrocks commentedThis certainly has changed over the last 2 years. I don't see why bartik, as a theme, can't have 2 special regions to host the primary and secondary menus. It is not an unusual practice of themers to place menus in their layouts via regions. The purpose of these regions is for layout, not functional. Seven doesn't use these menus, but Stark does, indirectly. Modules/System/Templates/page.html.twig still calls the old variables, even after the patch in #165, though a @todo was added. Stark inherits the menus from there. That seems to be more of a problem here.
Comment #191
RainbowArrayThis is my first stab at a revised version of this patch that relies upon regions rather than the header region.
I got an error on install with this patch applied so clearly something else still needs to be fixed. Hopefully this is a starting point at least.
When I created this patch, I actually stepped through and recreated it line by line, because I wanted to avoid missing some change that I didn't understand. It's possible I made an error somewhere while doing that. The interdiff will likely show if that happen.
I think one thing to look at is all the changes in the CSS to the header region. I'm not sure if all of those changes are necessary. Was that part of the thought that menus would go into the header region? If we're not doing that we may want to revert those changes.
Anyhow, hopefully this helps move this in the right direction. If somebody wants to take a stab at whatever awful errors testbot finds, please go right ahead!
Comment #193
Wim LeersLooks like the majority of the test failures (hopefully all!) are due to a simple mistake:
:)
Comment #194
RainbowArrayOh, duh. Whoops! That should be an easy fix. I may be able to roll a new patch for that today.
Comment #195
RainbowArraySo the upside is that this is now working. The menus are being placed in new regions. Yay!
The downside is that the appearance looks wrong, so there's more work to be done there.
Comment #196
RainbowArrayComment #198
Wim LeersI'll try to get this green again.
Comment #199
Wim LeersFor inexplicable reasons, the latest patch lost the following changes of mine:
PageCacheTagsIntegrationTest
PageCacheTagsIntegrationTest
, thereby causing failures inPageCacheTagsIntegrationTest
render element
insystem_theme()
was renamed torender_element
, thereby causing the failure in\Drupal\menu_ui\Tests\MenuTest
The failure in
BlockUiTest
was due to that test being very silly, unable to cope with new regions being added. Easy fix.This should be green again! :)
I'll now take a stab at fixing the appearance.
Comment #200
Wim LeersThis reroll brings appearance fixes.
overflow: hidden
, which causes the contextual links to be clipped. The only way to fix this, is to change the CSS quite drastically, which is out of scope for this issue. Let's fix contextual links in a follow-up — the fact that contextual links appear at all for the secondary menu is already an improvement.Attached: 3 interdiffs, applied in succession to go from #199 to this patch.
Comment #201
Wim LeersI don't think we should add regions to System module's
page.html.twig
? That implies every theme should support this, which is false. I think we should revert the changes to System module'spage.html.twig
, and only keep those in Bartik.Finally, I looked into doing this:
It's definitely possible. But that'll bring JS into this patch, which is hard to find reviewers for. Plus, it'll probably cleaning up
block.js
a bit, which has been unchanged since Drupal 7 except for coding style updates. So any change there is going to amount to a fair amount of bikeshedding probably. In any case, this can easily be a follow-up, because it requires no API changes. Opened #2343297: Add JS to prevent non-menu blocks from being added to Bartik's two special-purpose menu regions.Plus, that's not even absolutely necessary, as mdrummond said in #189:
Comment #202
dcrocks CreditAttribution: dcrocks commented(1)I built a new clone of D8.0 and the patch applied successfully. Everything looks OK except the new menu blocks don't appear on the 'Place Blocks' side bar. If an admin, for some reason, wanted to place an extra copy of these menus, or add them to a theme which currently didn't have them for some reason, they would be out of luck.
(2)All themes inherit blocks from Bartik when they are installed. If there is no corresponding region they are inserted into the 'default' region, usually the first defined region. So every theme added to an installation will gain those blocks via that route by default. Which would probably lead to some weird formatting unless the css didn't need the region definition and the css was in core css.
(3)It is currently common practice for themers to have a local copy of page.html.twig, which at this point still will likely refer to the old menu variables. That would make for an important change notice.
(4)I don't know how stark will get those blocks in minimal install and test scenarios unless they are in system's page.html.twig.
Comment #203
Wim Leers#202: Thanks for the review!
page.html.twig
and other templates already, so I'm not concerned there :)Comment #204
dcrocks CreditAttribution: dcrocks commentedI don't think anyone will want to make any changes to stark, otherwise it wouldn't be stark anymore. I tried a couple of contributed themes and the output didn't match bartik's, but themers should be able to fix that easily. And I noticed people surrounding their use of the old menu variables in local copies of page.html.twig with 'if' statements, so there actually may not be many issues there. I guess these are documentations issues more than anything else.
Also, if you don't have any regions specified in the theme's info.yml(e.g. stark) you get a default set of regions.
Comment #205
RainbowArray#199: Sorry I missed those. The perils of manually redoing a complicated patch.
#200: Thanks for fixing those. Before we RTBC, I want to give one more look at how we're handling the secondary menus now versus how we handled the header region before. The header region is still a valid place to put content, but I think some of the earlier changes overwrote some of the rules for the header region. Just want to make sure we don't have a regression for non-menu content.
#201: The page.html.twig template in system is also what's used in Stark, and yes that's inherited by contrib themes that don't override it. Again, to go back to what I said in #187, previously a site builder could choose a menu that would go into the primary menu area and another menu (or a second level of the same menu) that would go into the secondary menu area. By putting these regions in system/Stark, we're allowing site builders to do something similar. This wasn't a choice that was just available in Bartik. If a contrib theme wants to set up different menu regions or no menu regions at all, great, but I think we should aim to duplicate what could be done previously as much as possible.
#203.4: Did system/Stark have menus assigned to the primary and secondary menus by default? If so we should consider assigning the same menu blocks to the primary/secondary menu regions by default.
If we want to have follow-ups to discuss changing those defaults or whether or not there should be menu regions by default, I think that's fine. But I think that in this issue, we should be aiming to duplicate as much as possible what was available with the main/secondary menu variables that we're removing.
Comment #206
Wim LeersNo problem! If you manually redid this, wow, then you missed very little!
Yes! I forgot to do this. But you'll be able to judge this much better than I could, so: awesome! :)
That's a great reason, thanks for answering that! :)
This patch still assigns the same menus as before, so I don't think there's a problem there.
So what's left to be done here?
Comment #207
cosmicdreams CreditAttribution: cosmicdreams commentedHas a change record been started, but far from done: https://www.drupal.org/node/2343643
Comment #208
xjmJust a note that removing these functions is beta-deadline, so if this doesn't land before beta 1, we should consider what to do with them instead.
oopsie.
Comment #209
LewisNymanGreat work! I fixed few errors thrown up in csslint (see #2222049: Add a .csslintrc file that's in line with our CSS standards) and a few RTL errors.
Comment #210
Wim LeersThanks, Lewis! Am I correct if I assume that you reviewed the patch and think it's good to go from a front-end perspective?
I've refined the change notice further — thanks cosmicdreams for getting that started!
That leaves only the changing of the labels to match the menu names to be done. I'll do that later today.
Please review so we can get this to RTBC ASAP!
Comment #211
Wim LeersUpdated the issue summary to actually reflect the current patch.
Comment #212
Wim LeersChanges:
:)
Comment #213
RainbowArrayWe aren't removing the header region (nor should we) so I don't think these changes are probably necessary. It's still possible people could put blocks in the header region, so I don't even know that we should remove those menu styles for the header region..
I suspect that we'd want to look at these classes in particular. We should be able to change that to to page.secondary_menu ? at the beginning I'd think, so we could keep any shifts due to the presence of secondary menus the same.
Comment #214
Wim LeersI'm working on this, but it will be some time until I post a patch. Assigning to me to prevent duplicate work.
But there is zero CSS that uses this, so there's no value in keeping it around?
The only reason I haven't posted a patch yet, is because I also found regressions for the primary menu's appearance in the
narrow
andmobile
breakpoints. Fixing those also. The patch that I'll then post will hopefully be RTBC-worthy.Comment #215
Wim LeersThe CSS issues were caused by the changes made in the rerolls by either of you frontend folks :)
I'd carefully tuned the selectors to match the original intent:
#main-menu
targeted the<nav>
,#main-menu-links
targeted the<ul>
. In the new HTML structure, that means you need to target the<nav>
using.region-primary-menu .block-menu
and the<ul>
using.region-primary-menu .block-menu .menu
(or, alternatively, the shorter:.region-primary-menu .block-menu .menu
).Unfortunately, the latest CSS targeted the
<ul>
using.region-primary-menu .block-menu
, which is the one to target the<nav>
, not the<ul>
. While this works for some things, it doesn't work for everything. Notably,system.theme.css
stylesul.menu
and its children, and Bartik wants to override that.This probably happened because of the (great) goal to have shorter, simpler and therefore faster CSS selectors. But it was subtly wrong, hence causing subtle breakage.
In this reroll, I've made that wholly consistent with the old selectors again:
#main-menu
->.region-primary-menu .block-menu
#main-menu-links
->.region-primary-menu .menu
(short for.region-primary-menu .block-menu .menu
#secondary-menu
->.region-secondary-menu .block-menu
#secondary-menu-links
->.region-secondary-menu .menu
(short for.region-secondary-menu .block-menu .menu
A 1.5 minute annotated screencast to make it easier to verify all use cases work correctly
Screencast showing before/after (because otherwise you need to manually compare screenshots in 3 breakpoints, of the reference, the previous patch, and the current patch — it's easier to show/explain in a video):
stme
(simplytest.me), this is a Drupal 8 HEAD instance, i.e. the reference, the goal to match pixel-for-pixel.tres
, this is my patched local site (I have three local versions of Drupal 8: unus, duo, tres, to work on patches in parallel).tres
+ the patch in #212. You can clearly see the problems. Then I apply the changes for the patch attached to this comment. You can see how everything matches perfectly.bartik.wide
already correct + header region broken (00:05–00:15)bartik.narrow
: already correct (00:15–00:30)bartik.mobile
: already correct (00:30–00:20)stme
andtres
, plus reset them tobartik.wide
. Now we can restart our comparison.bartik.wide
still correct + header region fixed (00:55–01:05)bartik.narrow
is now correct (01:05–01:15)bartik.mobile
is now correct (01:15–01:30)Whew. Hope that helped.
interdiff-header.txt
shows the changes just to fix #213.1. The other interdiff shows the rest. That one is not very reviewable though. I optimized it to make the *actual patch* easier to review (least changes possible). So please review all the Bartik CSS changes not via the interdiff, but via the actual patch.(This is also why the new patch is smaller than the previous ones.)
Comment #217
Wim LeersStraight reroll.
Comment #218
Wim LeersI'm such a noob.
Comment #219
Wim LeersWe probably need to reroll this now that #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template and #1972122: Remove the DIV tag around block content have landed. But, that doesn't mean we can get a preliminary RTBC for #217 in the mean time, so that only the interdiff for a reroll will need to be reviewed still.
Comment #220
rteijeiro CreditAttribution: rteijeiro commentedRe-rolling...
Comment #221
rteijeiro CreditAttribution: rteijeiro commentedRe-rolled (I hope so)
Comment #223
lokapujyaI think there is some unnecessary spaces.
after .menu.
before and after body:not(:target)
* These were just minor; Doesn't mean there aren't bigger issues.
Comment #224
rteijeiro CreditAttribution: rteijeiro commented@lokapujya nice catches, thanks!
Tagging as novice and DrupalCon Amsterdam code sprint. Looking for a novice to contribute.
Comment #225
Wim Leers#221: is this a reroll to just chase HEAD or to address #219? I'm pretty sure it only chased HEAD?
#224: This is nowhere near a novice issue.
I'll get this green again.
Comment #226
lokapujyaComment #227
lokapujyaActually, summary looks ok.
Comment #228
Wim LeersReroll of #217, not #221.
Comment #229
joelpittetReviewed this with @Wim Leers in detail. And again after and I believe this patch is RTBC.
This patch provides:
<nav>
" tags for menu blocks.Next Steps:
There likely needs to be a followup for migrate to set new mappings for d6. (not sure how hard those would be to implements maybe they could be added to this patch even.
@see core/modules/migrate_drupal/config/install/migrate.migration.d6_menu_settings.yml
Thank you all!
Comment #231
webchickThis looks great!
I took this around the block (ha!) and back. Two issues I found:
#1:
Unfortunately this is a fairly obvious user-facing bug. It'd be awesome if there were some way to fix it tomorrow during the QA sprint.
#2: The menu block depth selector and such doesn't seem to be working. If I tell it to show two menu levels, and put "log out" underneath "my account" then I still only see just "My Account" when on that page, not the sub-page. However, this also is present in HEAD. But if we could make sure there's a follow-up for this (if there isn't one already) that'd be wonderful.
In the interest of making the most of the QA sprint tomorrow, I'd like to get this in now, even though it's not been RTBC very long. I feel like more end-users testing this out will lead to better results with future conversions.
Therefore!
Committed and pushed to 8.x. Yeeeeehawwwwwww!
Comment #232
joelpittetThanks @webchick!!!
Followup for issue #1 #2346515: Fix contextual links on new primary links/secondary links blocks
Followup for issue #2 #2346517: Menu blocks in Bartik's primary and secondary menu regions only show a single level, support multiple levels
And followup to get d6 migrate tests and schema for new primary/secondary menus.
#2346521: Get d6 migrate tests and schema for new primary/secondary menus back into core
Comment #233
RainbowArrayOh wow, amazing to see this get in! It has been a very long road! Thanks for all the work from everyone to get this in!
Comment #234
tim.plunkettThis is slightly broken.
{% set heading_id = attributes.id ~ '-menu'|clean_id %}
attributes.id is optional, in tests I'm seeing
<nav id="-menu"
which is wrong.I have no idea what ~ does, or why we're not just doing this in preprocess, but can someone open an issue?
Comment #235
joelpittet@tim.plunkett
~
is concatenation in twig. Dot operator is used for objects and + operator for math... so they went with~
Was the id defaulted because in twig you could do this:
Also that clean id is quite useless... due to operator precedence:
https://github.com/fabpot/Twig/issues/328
It should be
And the finally, any questions regarding "why not doing things in preprocess" need to be routed to @JohnAlbin, @mdrummond, @davidhernandez and of course @mortendk
They are the banana experts... and are much better at explaining the plan.
https://www.drupal.org/node/2322163
@mdrummond do you mind opening that issue to fix that up?
Comment #236
Wim LeersStrange, any chance they're only broken in tests? It works fine in the actual rendered pages for me (if it didn't, then the main menu toggle on very narrow viewports wouldn't work either), at least in
6ef8ba79091ba8ef07008a68ae11c8fcde7d1958
.Comment #237
RainbowArrayWe're not doing class declarations for templates in preprocess anymore, but this is an id, so slightly different. If the heading ID is optional, then we could tweak the template to fix that. Would be good to know if this is a test-only issue, though?
Comment #238
joelpittetIt is here, not in a test. :) We could like add a test to show that is broken in the fix up issue, I don't mind helping with that if you want? Just need to pass an unclean attribute.id to the template (all attribute key/values checkplained so not that kind of unclean). Likely should show both a fail and a success for the menu block.
Comment #240
LewisNymanFYI, I've created a followup to fix a usability regression introduced in this issue #2513526: Rename the menu regions