Problem/Motivation
Menu classes are generated in menulinkthree.php instead of beeing variables that the theme then uses and creates the classes that it want
its cumbersome for a theme to change this by ex using .hasClass()
set class = [
'menu-item',
item.attributes.hasClass('menu-item--expanded') ? 'expanded-to-max' : 'no-expandin-for-you',
item.attributes.hasClass('menu-item--collapsed') ? 'collapsed' : 'not-collapsed',
item.attributes.hasClass('menu-item--active-trail') ? 'ze-trail-mybroherts' : 'no-trail-dude',
]
Proposed resolution
remove the class array and exchange that with relevant variables.
if ($data->hasChildren && !empty($data->subtree)) {
$class[] = 'menu-item--expanded';
}
to this
if ($data->hasChildren && !empty($data->subtree)) {
$element['is_expanded'] = TRUE;
}
template:
item.is_expanded ? 'menu-item--expanded'
Similar changes have been made to set variables is_collapsed and in_active_trail for both the menu and book-tree templates.
It gets visible for the themer where & what gets generated & we get the true separation of code & templates
Remaining tasks
- Review and commit
User interface changes
- None
API changes
- Adds three additional preprocess variables
Data model changes
- None
Beta phase evaluation
Issue category | Task because this is primarily an improvement for where markup is generated in line with other changes to the theme system. |
---|---|
Issue priority | Normal because this is a useful change for themers, but should not block release. |
Disruption | Not at all disruptive as the template output remains the same. |
Comment | File | Size | Author |
---|---|---|---|
#56 | interdiff-menugen-51to56.txt | 835 bytes | davidhernandez |
#56 | menu_class_generating-2547159-56.patch | 15.85 KB | davidhernandez |
#51 | interdiff-2547159-45-51.txt | 5.95 KB | RainbowArray |
#51 | 2547159-51-menu-class-generating.patch | 15.84 KB | RainbowArray |
Comments
Comment #2
mirom CreditAttribution: mirom at i-KOS commentedComment #3
mortendk CreditAttribution: mortendk as a volunteer commentedok testbot lets see how you like this
Comment #4
mirom CreditAttribution: mirom at i-KOS commentedComment #8
lauriiiComment #9
joelpittetCouple questions and notes:
Should this only go in classy?
If this is to move back the whole
<li>
should be unindented as well.Comment #10
Wim LeersI'd have expected these to be
#is_expanded
etc., i.e. properties. But I guess that doesn't make sense here?Comment #11
joelpittet@Wim Leers they aren't render arrays they are just item hashes, the $element variable name is just there to throw you off your game;)
Comment #12
Wim LeersIndeed!
Just ignore #10, #11, #12 please — sorry for the distraction!
Comment #13
mortendk CreditAttribution: mortendk as a volunteer commented@joel yes it should only be in classy (and in the menu--toolbar) it is in classy where the classes should live.
Comment #14
RainbowArrayIn #2553463: Move system link.theme.css, links.theme.css, menu.theme.css, and tabs.theme.css to Classy, we found there's still a menu class in menu.html.twig in core. It looks like the associated styles are purely aesthetic. Trying to figure out the best place to take care of moving that class to Classy. Unless there's a specific reason that's still in core?
Comment #15
mortendk CreditAttribution: mortendk as a volunteer commentedas I remember it the reason we still have .menu was some test issue ?
if that's not the case then everything goes to classy - but that's not this issue, this is about moving hardcoded classes to variables ;)
Comment #16
Wim LeersNote "in" versus "is". Should be consistent.
Comment #17
RainbowArrayI figured out the menu class issue. Not relevant here. Yes, it was in the template, we pulled it out for a test issue, we fixed the test but never pulled that class out. That's being fixed in another issue.
This fixes a typo, the in vs is thing Wim pointed out and uses a classes vs class variable, which looks to be the pattern in other templates.
I just spent too many hours fighting problems just like this in D7. This looks like a great fix, it's straightforward, and I think we can move to RTBC on this one. Let's get this in!
Comment #18
RainbowArrayAlso, it's right to keep those classes in menu--toolbar for core, as those need to be available to non-Classy-based themes as well, so the toolbar works everywhere.
Comment #19
davidhernandezThis is basically a boolean state, so why not just do something like:
Trying to think if that variable is useful for something more than just making a class.
Comment #20
RainbowArraydavidhernandez: I think state is a pretty generic thing where it's not at all clear what is in that. is_expanded and is_collapsed is very clear and will be easier to use by themers without having to look up the possible values of state, or the possibility that some other random thing gets thrown into the state variable because somebody is not clear on what it should be used for.
Comment #21
mortendk CreditAttribution: mortendk as a volunteer commentedthe reason I used is_ is to keep it in the same feel as smacss have that use is for its state
Comment #22
RainbowArray#21: +1
Comment #23
joelpittetDon't remove this, it resets the element in the array.
I think this looks good otherwise. I don't care either way on thie
is_
vsin_
. So sort it out between yourselves.Comment #24
lauriiiIts just moved few rows up.
Comment #25
RainbowArrayWe didn't remove $element = array(), it just got moved up a little higher in the function.
Comment #26
joelpittet@mdrummond mentioned it moved up. (should have looked up).
Anyways this looks good as-is.
Comment #27
joelpittetEdit: Moar context
Indent here needs to be moved in one like other one.
Also more importantly could we expand this issue to include
\Drupal\book\BookManager.
?It has identical code or can someone make a follow-up?
Comment #28
RainbowArrayThis does the same thing for book-tree that we were doing for menu and fixes the indent too.
Comment #29
joelpittetProbably don't need to delete this comment.
Nice find! That definitely needs to be in this issue.
I think you were right to move the for back but the end for needs to be inline with it still. It looks silly because the if/else above it indents the ul.
Comment #30
RainbowArrayComment #31
RainbowArrayComment #33
RainbowArrayRedid the patch. Something odd may have happened.
Comment #36
joelpittetSo question, since the other ones don't touch core/stark should this template be duplicated in classy instead?
I'm ok with leaving it like this because technically that's what's in core right now. But thought I'd ask.
classy?
Comment #37
RainbowArrayNo because the toolbar needs to work in non-classy themes. So all themes need those classes.
Comment #38
star-szrSorry to kick this back but the fixes are easy.
Minor: I guess we can use short array syntax on the first one too.
I thought elsewhere we were initializing these as FALSE for easier debugging.
Example from template_preprocess_views_view_table():
If we remove all uses of $classes from this function thingy then we should probably remove it from the $get_built_element line.
Comment #39
star-szrComment #40
RainbowArrayThis should fix the items in #38.
Comment #43
RainbowArrayTested this locally, seems to works now. We removed the classes array from the function definition but not from where the function was called. That's fixed now. Also set up the defaults in the unit test as well.
Comment #44
star-szrThanks @mdrummond! I think below is all I can find now and then this is ready from my perspective after it gets an issue summary update (and beta evaluation).
This is a bit terse, can we say something more like: "…whether the link is in the active trail."
(would need to be wrapped to 80 chars).
Again a bit terse, "…only set 'expanded' to true…" would be an improvement I think.
Let's document these three new sub-variables in all relevant menu and book-tree templates please.
Comment #45
RainbowArrayThis should fix the doc changes requested in #44.
Comment #46
RainbowArrayComment #47
RainbowArrayComment #48
dawehnerI really like the patch as it is. Its one small step from moving towards view objects.
Comment #49
davidhernandezDoing some testing.
Comment #50
davidhernandezI'm mostly testing in Classy. Stark has no classes for these templates so the only difference I see is whether the right comments have been added to the templates.
Enable book and generate book content. Added nested book items.
Enable Classy and add Administration menu block and Book navigation block.
Modify Admin menu to have several items expanded and collapsed.
Checking markup and classes added. Copied the markup for both and toolbar menu for reference.
Verify that all three menu types have expanded, collapsed, and active trail.
Apply patch.
Checking book menu, no difference.
Checking admin menu, no difference.
Checking toolbar, no difference.
I diffed the output from all three and did not find a difference. Since the class generation is the same, CSS shouldn't be affected.
A couple things I noticed, none are big.
1) I noticed the toolbar never sets active trail. Is this a bug or by design? It is that way in head, so not something fix here. Just something I noticed.
2) The indentation of the templates is inconsistant in one regard. The for loop after the endif is indented in all the different templates, but unindented in Classy's menu template. Unindented is actually correct. Regardless of wich way, we should make them all the same.
3)
Why is this hyphen here after the li tag? It's in the original, but I don't think I've seen it written that way anywhere else.
Otherwise, I'm inclined to RTBC it.
Comment #51
RainbowArrayThis fixes the issues mentioned in #2 and #3. As #1 indicates, if the active trail wasn't being set in the toolbar prior to this patch, I don't think we'd want to address that here. I'm not sure you would want an active indicator in the toolbar, but either way, that does not seem in scope here.
Consistency is in scope, though, so thanks for the finds, davidhernandez! Much appreciated.
Comment #54
RainbowArrayComment #55
davidhernandezHmm. One nitpick. The comments are not the same between the two menu templates. System and Classy. It looks like one was maybe copied from the book template and the work book was removed, leaving the sentences different.
In the system template it specifies menu tree.
Here it does not. The toolbar template also says "menu tree".
Comment #56
davidhernandezHere is the text change. If it's ok I'll rtbc.
Comment #57
RainbowArrayGreen, so rtbc based on above comment.
Comment #60
RainbowArraySeems like a random unrelated test fail.
Comment #61
RainbowArrayDrupal CI is green. Moving this back to RTBC.
Comment #62
webchickCommitted and pushed to 8.0.x. Thanks!