FIX Flyout cms menu panels

[jonom]

The hover flyout functionality for nested CMS menu items seems to be broken in 3.3. Seems to be failing because behaviour was added with regular jquery (just once at runtime) rather than using entwine.

[tractorcow]

I had no idea that hover flyout was a thing until now. :P

[jonom]

Yeah I only found out about it by accident as it's used by https://github.com/silverstripe-australia/silverstripe-grouped-cms-menu :) I'm not sure the behaviour is documented or belongs in framework really...

[jonom]

ping @tractorcow any thoughts on this one?

[stevie-mayhew]

@jonom I have a thought!

I think that it should be removed from framework, and replaced in the grouped-cms-module. Ideally this would happen in reverse, but at the moment it is code which we have to use for grouped-cms-module and have no control over, so it would be good to have control where its used.

IMHO CMS grouping should be core though, I'd be happy to write the code to support it for 4.0.

[jonom]

I think that it should be removed from framework, and replaced in the grouped-cms-module

@stevie-mayhew that would be my instinct too, but it wouldn't be very SemVer for this feature to disappear in 3.3 would it? There could be other people out there using it.

I would have suggested removing it for 4.0 and moving the javascript to the grouped-cms-module but I agree with you that grouping should be a core feature so would much prefer that :) I find it very useful even on the most basic sites.

[stevie-mayhew]

Yeah definitely a 4.0 thing. @clarkepaul do you have any plans for this in 4.0 or should I just have at it?

[clarkepaul]

I've started updating the styles for the main menu as part of a hackday thing and haven't gone as far as doing anything for the nested level or any testing. For grouped-cms-module I'm still a bit unsure of what is in Framework or in the module but my intention was to continue the styles to the nested items. If there was to be any updates to the functionality then now would be perfect timing. There are no plans for the team here to do any work on the nested nav so if you guys could manage to get it ready (and I guess approved) then I could do the fine tuning.

@chillu is this something which could accepted into core 4.0? To me this feels like the way to go, it provides more flexibility for the user experience.

Screenshot of WIP:

[jonom]

Glad to see this getting considered as a core feature! In the mean time though, appreciate if anyone can review this PR as a fix for the broken functionality in 3.3 - would be good to have it ready for a 3.3.1 release.

[chillu]

Don't have any strong opinions here, but my feeling is that it shouldn't be in core if it can be cleanly supported through a module - which is what we're trying to achieve with React and component based styles. I don't think all CSS and JS behaviour can be considered part of the public API in terms of semver. It's a good goal to aspire to, but where do you draw the line there? So for the existing grouped-cms-menu, is it possible to add CSS and JS to the module in order to make it work with 3.3?

Back story from 3.0: Felipe designed his 3.0 screens with a nested menu, but we only got around implementing the CSS/JS side of it. The CMSMenu class (from 2.x days) with its coupling to routing made it hard to transition into an API with properly nested CMS menus. Hence the "hidden feature".

[jonom]

Okay so probably not a SemVer violation if we don't patch it. Yes, should be possible to fix this in grouped-cms-menu instead.

Rather than just close this PR, should I change it to remove the broken code instead of fix it? Should prevent conflicts and no point leaving in something that doesn't work right?

On whether or not this should be a core feature for 4, my point of view is that we already support hierarchy / deep nesting for menus in individual sections (via form tabs), so it feels like we should provide the same level of support for the main menu. So if not a core feature I think it is a good candidate to be one of the SS Ltd officially supported modules at least.

[stevie-mayhew]

Totally agree with @chillu about making things more modular, and 👍 to what @jonom says about it being something potentially supported as a "approved module" or whatever it is.

We've had no issues with it in 50 or some stupid number of sites so I'd consider it pretty stable. Happy to upgrade and pull out the functionality relied on from SS core currently to make it standalone.

[jonom]

@stevie-mayhew are we both talking about this module https://github.com/silverstripe-australia/silverstripe-grouped-cms-menu or does Little Giant have something similar? :D either way very happy for you to have at it!

[stevie-mayhew]

@jonom we are both talking about the same thing 👍

Should we sneakily make the entire menu system React?

[chillu]

@stevie-mayhew I think that would actually be a great start to getting into the SilverStripe-way of writing React components, and validate our approach. So you could rewrite LeftAndMain.Menu.js to React in core, then modify those components to support nested menus. See #4911 and #4887 as a starting point. You'll need to work off our team branches at the moment though in terms of latest React integration:
https://github.com/open-sausages/silverstripe-framework (branch: integrate-react)
https://github.com/open-sausages/silverstripe-cms (branch: integrate-react)

/cc @flashbackzoo

[jonom]

👍 Woohoo that would be great @stevie-mayhew! You know if you do decide to do this and keep some notes while you're working on it, it might make a great blog post. I have no React experience currently so would be really interested to read through your process and any gotchas etc. you encounter, and as @chillu says would be a great way to validate the approach. Could be a good case study on how to adapt components from SS3 to SS4.

[flashbackzoo]

@vinstah was trying to finish off the flyout menus (using Entwine) last week.

I think trying to implement the menu in React, at this stage, will lead to much frustrations. There are a lots of interactions with Entwine there, which can be managed, but would be better attacked further down the track. @chillu remember the fun we had with GalleryField last year? 😜

The asset admin module https://github.com/open-sausages/silverstripe-asset-admin/tree/pulls/add-routing might be a better place to start hacking.

/cc @ianherbert

[jonom]

Where are we at with this guys, should I update this PR to remove redundant/undocumented code or just close it?

[clarkepaul]

I've styled the nested nav items to go with the newer v4 styles of the menu and @scott1702 was going to squash it for me. @scott1702 where is that going to get merged into? I take it my changes could then be removed into the module itself.

For my design it would work best if the group had the icon (rather than the nested items) but I wasn't sure how to set that up.

[jonom]

Yeah I really like that design with an icon on group only. Looks quite cluttered currently with an icon for each child. I added icon support for the grouped cms menu module recently but did it in a bit of a dumb way - I didn't realise there was an API for setting icons on LeftAndMain menu items so would be good to make it more consistent.

That's probably something to consider either for 3.4 or 4 depending on where those updates are headed, in the mean time the grouped-cms-menu module needs a fix to be compatible with 3.3. @stevie-mayhew did you have time to have a look at that?

Will close this since we seem to have decided to fix in module instead of here.

[clarkepaul]

Made a merge request for the styles #5161

[jonom]

Hi guys, I'm re-opening this because I think it should be considered a bugfix after all. We know at least one popular module relies on this functionality and see this description from the top of LeftAndMain.Menu.js:

Vertical CMS menu with two levels, built from a nested unordered list.
The (optional) second level is collapsible, hiding its children.
The whole menu (including second levels) is collapsible as well,
exposing only a preview for every menu item in order to save space.
In this "preview/collapsed" mode, the secondary menu hovers over the menu item,
rather than expanding it.

I would say that's more documented than hidden, so removing the flyout functionality in 3.x seems anti-SemVer to me - we could leave it untouched here of course but then it's difficult to fix this in the grouped-cms-menu existing behaviours in Framework could cause conflicts. The functionality is currently broken for me on some sites but not others - I can't trace the reason for this discrepancy but this updated code works everywhere for me.

Happy for this to be removed from 4 although I do think grouping menu items together is a pretty basic feature, and we allow this within form interfaces so why not the main menu? What I think would be really cool is if CMSMenu and FieldList shared a common interface and API for nesting and reorganising items. Think it would be really useful if we could choose a field or menu item and say 'move to this tab', or grab a tab/section and say 'put this in front of this other tab'.