-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#8752: Hide/show menu items on the frond-end #8753
base: 1.10.x
Are you sure you want to change the base?
#8752: Hide/show menu items on the frond-end #8753
Conversation
# Conflicts: # src/Orchard.Web/Core/Navigation/Services/DefaultMenuProvider.cs
|
||
if (!Services.Authorizer.Authorize(Permissions.CreateContent, contentItem, T("Couldn't clone content"))) | ||
if (!Services.Authorizer.Authorize(Permissions.ViewContent, originalContentItem, T("Couldn't open original content"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why these permissions are changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We started from a fork version of LaserOrchard which already has these changes, it is a part of code that I did not write but which I had to integrate to remain aligned with the version we started from. Ours mod is just a proposal, which unfortunately integrates changes that were not made by us, if too different from the code, as far as I'm concerned it can be rejected entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are from the dev branch.
@@ -21,6 +21,7 @@ | |||
using Orchard.Core.Contents.Settings; | |||
using Orchard.Data; | |||
using System.Web.Routing; | |||
using Orchard.Time; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
@@ -27,7 +28,23 @@ public class DefaultMenuProvider : IMenuProvider { | |||
.Where(x => x.MenuId == menu.Id) | |||
.List(); | |||
} | |||
var menuParts = _menuPartsMemory[menu.Id]; | |||
var menuParts = _menuPartsMemory[menu.Id].ToList<MenuPart>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToList is unnecessary. Change the type of the dictionary instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you think the enumeration is executing the query again.
var menuParts = _menuPartsMemory[menu.Id].ToList<MenuPart>(); | ||
|
||
//List of hidden items | ||
var menuPartsHidden = _contentManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The menus are already loaded from database, why do another query? You can just filter in Linq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but the code didn't return the list I expected, this is the committed code I added in my version
//An attempt to optimize the code above but unsuccessful
//var menuParts = _contentManager
// .Query<MenuPart, MenuPartRecord>()
// .Where(x => x.MenuId == menu.Id && x.VisibleAtFrontEnd && !_contentManager
// .Query<MenuPart, MenuPartRecord>()
// .Where(x => x.MenuId == menu.Id && !x.VisibleAtFrontEnd)
// .List().Where(w => x.MenuPosition.StartsWith(w.MenuPosition)).Any())
// .List().ToList();
So it's really just to be able to hide a menu item without deleting it? So just "Visible" or "Hidden"? |
Please see my comments in related issue #8752 |
/// </summary> | ||
/// <param name="menu">The menu item</param> | ||
/// <param name="value">Show->true or Hide->false</param> | ||
private void VisibleAtFrontEnd(MenuPart menu, Boolean value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this method be here? It's working on MenuPart, but this controller is the one for ContentItems
Wouldn't publish/unpublish be able to achieve the same functionality (with hopefully less code)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gdessimoneinvalle @MatteoPiovanelli-Laser please see #8807 for a simpler implementation based publishing/unpublishing menu items. Does this satisfy your requirements?
Fixes #8752
We propose the solution we have developed to the community's attention