Reorganize the Admin Menu #17112
Reorganize the Admin Menu #17112
Conversation
|
This is the first time I have seen |
|
Please read the referenced issue. |
No |
|
Look at the discussions. Tools is a better term to use here since we added the Settings. Do |
|
I like it, but I'm not sure about moving audit trail. I think it's important enough to stay at the root. While you are at it, do we agree to move GraphiQL to Tools, or should it go to search as requested in #12710? I think it makes sense in the new tools menu. |
|
Additionally, can we somehow group authentication and security settings? They seem to be closely related IMHO. |
This was requested by @sebastienros in one of the meetings. I prefer keeping the main menu free for users to utilize as they see fit. While the importance of features is clear, it can vary greatly from one user to another. So, why should the Audit Trail take priority over other features like Workflows, the Admin Menu, Features or Deployments?
I do not disagree with you there and Authentication is part of security. I am just trying to not add yet another level to the depth of the menu. and is one of the reasons why I went down an alternative approach here so that the main menu is much thinner and not too deep. |
Thanks, I agree with all you said. |
|
@gvkries I moved the I also renamed the "Security" tab at the root level to "Access Control." This feels like a much better fit, especially since the security settings have been moved to the Settings tab. The term "Security" can often feel overused, and "Access Control" provides a clearer, more specific representation of the functionality related to users, roles, and authentication. |
MikeAlhayek
added
the
breaking change 💥
|
In general, I'm OK with this. However, I see frustration both for users (to learn and unlearn the new menu structure) and developers (basically all admin menus will need to be checked, most changed) when getting this update. Documentation needs to be updated all over the place as well. All of these are subtle breaking changes one will only discover when bumping into an issue or users complaining, not instantly clear like a build error. I don't know how to make this easier. For sure it starts with documenting it, and providing a search & replace pattern for the simple case of Configuration -> Settings, so you can at least quickly fix that in the codebase. |
|
Once we all agree on the new menu, I'll update the documentation and add release notes updates. We'll probably talk more about it during triage tomorrow. I do agree it's going to require every project to change. But the good news that it's part of 3.0 :) |
|
If we could not screw users for some time that would be great. There are breaking changes that don't break anyone or are super natural to fix (
This doesn't sound good. It's not because we had to ship some breaking changes that we can add any other ones. Every breaking change should be done with measure. |
|
@sebastienros We could complicate things and introduce a switch to keep old behavior. But if we do that, the code will double as we'll need to check the switch and either load old menu or old menu. Alternatively, we could create a second menu "admin" and "admin-new" so if the user want to stay with prior state, then we would load admin-new instead of "admin" |
|
I've tried this out and I love it :-) Some things I would like to mention:
Thanks @MikeAlhayek |
|
@gvkries if you can, please checkout the PR and test drive the changes. Here is a recipe that would enable all the feature that I am using in you or someone else want to test drive {
"steps": [
{
"name": "Feature",
"enable": [
"Application.Default",
"OrchardCore.Cms.Web",
"OrchardCore.Settings",
"OrchardCore.Admin",
"OrchardCore.Liquid",
"OrchardCore.Contents",
"OrchardCore.ContentTypes",
"OrchardCore.Shortcodes",
"OrchardCore.Html",
"OrchardCore.Recipes.Core",
"OrchardCore.Title",
"OrchardCore.AdminMenu",
"OrchardCore.Templates",
"OrchardCore.AdminTemplates",
"OrchardCore.Alias",
"OrchardCore.Apis.GraphQL",
"OrchardCore.ArchiveLater",
"OrchardCore.AuditTrail",
"OrchardCore.Autoroute",
"OrchardCore.Scripting",
"OrchardCore.Recipes",
"OrchardCore.BackgroundTasks",
"OrchardCore.ContentFields",
"OrchardCore.ContentFields.Indexing.SQL",
"OrchardCore.Roles.Core",
"OrchardCore.Users",
"OrchardCore.ContentFields.Indexing.SQL.UserPicker",
"OrchardCore.Localization",
"OrchardCore.Users.Localization",
"OrchardCore.ContentLocalization",
"OrchardCore.ContentLocalization.ContentCulturePicker",
"OrchardCore.Sitemaps",
"OrchardCore.ContentLocalization.Sitemaps",
"OrchardCore.ContentPreview",
"OrchardCore.Deployment",
"OrchardCore.Contents.Deployment.Download",
"OrchardCore.Contents.Deployment.ExportContentToDeploymentTarget",
"OrchardCore.CustomSettings",
"OrchardCore.Deployment.Remote",
"OrchardCore.Diagnostics",
"OrchardCore.DynamicCache",
"OrchardCore.Email",
"OrchardCore.Email.Azure",
"OrchardCore.Email.Smtp",
"OrchardCore.Facebook",
"OrchardCore.Users.ExternalAuthentication",
"OrchardCore.Facebook.Login",
"OrchardCore.Facebook.Pixel",
"OrchardCore.Widgets",
"OrchardCore.Facebook.Widgets",
"OrchardCore.Resources",
"OrchardCore.Features",
"OrchardCore.Feeds",
"OrchardCore.Flows",
"OrchardCore.Forms",
"OrchardCore.GitHub.Authentication",
"OrchardCore.Google.Analytics",
"OrchardCore.Google.GoogleAuthentication",
"OrchardCore.Google.TagManager",
"OrchardCore.HealthChecks",
"OrchardCore.HomeRoute",
"OrchardCore.Https",
"OrchardCore.Indexing",
"OrchardCore.Rules",
"OrchardCore.Layers",
"OrchardCore.Lists",
"OrchardCore.Localization.AdminCulturePicker",
"OrchardCore.Localization.ContentLanguageHeader",
"OrchardCore.Markdown",
"OrchardCore.Media",
"OrchardCore.Media.Cache",
"OrchardCore.Media.AmazonS3",
"OrchardCore.Media.AmazonS3.ImageSharpImageCache",
"OrchardCore.Media.Azure.ImageSharpImageCache",
"OrchardCore.Media.Azure.Storage",
"OrchardCore.Media.Indexing",
"OrchardCore.Media.Indexing.OpenXML",
"OrchardCore.Media.Indexing.Pdf",
"OrchardCore.Media.Indexing.Text",
"OrchardCore.Media.Security",
"OrchardCore.Media.Slugify",
"OrchardCore.Menu",
"OrchardCore.Microsoft.Authentication.AzureAD",
"OrchardCore.Microsoft.Authentication.MicrosoftAccount",
"OrchardCore.Navigation",
"OrchardCore.Notifications",
"OrchardCore.Notifications.Email",
"OrchardCore.Sms",
"OrchardCore.Notifications.Sms",
"OrchardCore.OpenId",
"OrchardCore.OpenId.Client",
"OrchardCore.OpenId.Management",
"OrchardCore.OpenId.Server",
"OrchardCore.OpenId.Validation",
"OrchardCore.Placements",
"OrchardCore.PublishLater",
"OrchardCore.Queries.Core",
"OrchardCore.Queries",
"OrchardCore.Queries.Sql",
"OrchardCore.ReCaptcha",
"OrchardCore.ReCaptcha.Users",
"OrchardCore.XmlRpc",
"OrchardCore.RemotePublishing",
"OrchardCore.ReverseProxy",
"OrchardCore.Roles",
"OrchardCore.Search",
"OrchardCore.Search.Elasticsearch",
"OrchardCore.Search.Elasticsearch.ContentPicker",
"OrchardCore.Search.Elasticsearch.Worker",
"OrchardCore.Security",
"OrchardCore.Seo",
"OrchardCore.Shortcodes.Templates",
"OrchardCore.Sitemaps.Cleanup",
"OrchardCore.Sitemaps.RazorPages",
"OrchardCore.Sms.Azure",
"OrchardCore.Taxonomies",
"OrchardCore.Taxonomies.ContentsAdminList",
"OrchardCore.Themes",
"OrchardCore.Twitter",
"OrchardCore.Twitter.Signin",
"OrchardCore.UrlRewriting",
"OrchardCore.Users.2FA",
"OrchardCore.Users.2FA.AuthenticatorApp",
"OrchardCore.Users.2FA.Email",
"OrchardCore.Users.2FA.Sms",
"OrchardCore.Users.AuditTrail",
"OrchardCore.Users.ChangeEmail",
"OrchardCore.Users.CustomUserSettings",
"OrchardCore.Users.Registration",
"OrchardCore.Users.ResetPassword",
"OrchardCore.Users.TimeZone",
"OrchardCore.Workflows",
"OrchardCore.Workflows.Http",
"OrchardCore.Workflows.Session",
"OrchardCore.Workflows.Timers",
"SafeMode",
"TheAdmin",
"TheBlogTheme"
]
}
]
} |
|
"Design" is very much about look and feel for me, so I wouldn't put Content Definition there; that's good under Content (I wouldn't move it, but if everybody is against it staying, then at least Settings). Media, however, I'd consider either content (it is content, though it's not content items) or a top-level item. However, it has always bothered me that we have Content -> Content Types, and it's not actually about content types (and not to be confused with Content -> Content Definition -> Content Types), but content items filtered by content types. Perhaps "Items by Type"? |
|
@Piedone hopefully you can watch the meeting soon |
That is what I thought at first. However, it contains other things like shortcodes and placements. If these were not there, they I would have suggested it to name it "Appearance" instead of "Design". That said, "Design" is more than Appearance, so we thought its better to move the content definition "which is a content design" and keep the menu called "Design". @sebastienros even suggested to move Workflows into Design too which I don't mind. But @gvkries did not like that idea. Thoughts on that? Workflow can fit under tools and under Design.
This is part of the blog recipe and can be changed. It's not part of the default navigation providers. We can always put that one level up instead of it being in "Content types" as well.
It is currently on the top level and I find it much better this way. So we agree on the media being top level. |
|
OK then! Please fix the Blog recipe too, then. Workflows is much more Tools for me than Design. |
|
@Piedone I updated the blog recipe. I am not sure why we were showing all content types on the menu. Not very helpful. It was just extra clutter. I updated it to only show Articles, Pages, and Taxonomy.
|
|
Looks good! |
|
@Piedone @sebastienros @gvkries Any objections to merging this PR? I'd like to give everyone some time to familiarize themselves with the new structure and potentially make any necessary updates or suggest further improvements we might have missed. Once we release a new preview with these changes, I hope it will prompt more feedback (if any) from the community. |
| @@ -22,18 +22,34 @@ public AdminMenu(IStringLocalizer<AdminMenu> stringLocalizer) | |||
|
|
|||
| protected override ValueTask BuildAsync(NavigationBuilder builder) | |||
| { | |||
| builder | |||
| if (NavigationHelper.UseLegacyFormat()) | |||
There was a problem hiding this comment.
Add to the release notes about this PR, as well as how to opt back to the legacy format. The documentation also needs to be combed through for any references to the menu structure, either in written instructions, or images.
There was a problem hiding this comment.
Planning to update the docs before merging. Just need to finalize the PR first.
| builder | ||
| .Add(S["Configuration"], configuration => configuration | ||
| .Add(S["Tools"], tools => tools |
There was a problem hiding this comment.
To me, Tools are functionality that act on the system on a technical level, and aren't about just changing settings. Features is the latter category, as well as admin menus.
There was a problem hiding this comment.
I strongly disagree with your suggestion to moving Feature and Admin Menu to the Settings menu. I think they should stay under tools. These are better fit for tools. + we don't need to treat everything are settings and clutter the settings menu more.
There was a problem hiding this comment.
Why is toggling feature states more of a tool than configuration?
There was a problem hiding this comment.
It's a tool to enable/disable features. It's not purely settings. We should not just start moving things into settings just because they make changes. We should only put settings related items under settings.
There was a problem hiding this comment.
Isn't the configuration page to change the site's title also but a tool to set the title? Or would a different features UI, like with a list of checkboxes for each feature and a Save button at the bottom, make it more settings-y? :)
There was a problem hiding this comment.
To me, if we have a form to fill out or change settings, that would be under the Settings. Features, its a tool to manage features.
There was a problem hiding this comment.
What if I tell you that the Features page is actually a form already? :)
My point is, we have to distinguish between these based on the underlying principle and not a perception of the current UI, and toggling feature states is changing configuration, i.e. settings, in the same way as changing the site title is.
There was a problem hiding this comment.
If you strongly feel it should be in Settings while I strongly feel it should be in Tools, I think we should get other opinions on this. We'll discuss it more in the Steering meeting today.
There was a problem hiding this comment.
@Piedone talked about this in the meeting. @sebastienros and @gvkries suggested to keep it in Tools as well.
src/OrchardCore.Modules/OrchardCore.Media.AmazonS3/AdminMenu.cs
Outdated
Show resolved
Hide resolved
| .Permission(Permissions.ManageEmailSettings) | ||
| .LocalNav() | ||
| ) | ||
| .Add(S["Email test"], S["Email test"].PrefixPosition(), entry => entry |
There was a problem hiding this comment.
I don't think I'd look for sending a test e-mail under Tools -> Testing -> Email test after configuring email settings under Settings -> Communication -> Email.
There was a problem hiding this comment.
It's a tool to test email or SMS service.
There was a problem hiding this comment.
Arguably yes, but nobody will think about it like this when setting up e-mail (or SMS) delivery. You want to test e-mail sending after changing the e-mail config.
There was a problem hiding this comment.
Well, that would be the first thing I would look under. I would never guess that a service test would reside under settings.
There was a problem hiding this comment.
Your first thought would be to look four steps elsewhere in the menu structure, under an item named "Testing"? Perhaps if you configure e.g. SMTP settings in appsettings (i.e. not under Settings -> Communication -> Email) then it's better, but it's not at all intuitive when working with the settings from the admin. A notification displayed after saving the e-mail/SMS settings along the lines of "Do you want to verify if these settings work? Send a test email ."
"Testing" is not my favorite in itself, I'd first understand it as having something to do with automated testing like unit tests. Having a Tools -> Communication submenu too seems better.
There was a problem hiding this comment.
Testing would be for any kinds of testing. At least that was the idea. So if we have more service test tools, we can place them under the testing.
There was a problem hiding this comment.
If we'll ever have those, most possibly they'll more appropriate in a more expressive submenu too.
There was a problem hiding this comment.
I feel "Testing" menu here is a great fit. If you have a batter generic name, I am okay with it. But these should not be moved into settings.
At a high level, any UI that users OrchardCore.Settings should be put in Settings along with readonly settings like the media options. Any tool like testing, url rewriting, features, etc would be a tool.
| builder | ||
| .Add(S["Settings"], settings => settings | ||
| .Add(S["Security"], S["Security"].PrefixPosition(), security => security | ||
| .Add(S["Authentication"], S["Authentication"].PrefixPosition(), authentication => authentication |
There was a problem hiding this comment.
Rather External Authentication, for the others too.
There was a problem hiding this comment.
I would rather keep it Authentication. Why would we call it external authentication where is this the other authentication item we have in the menu. The other thing have is Access control
There was a problem hiding this comment.
The only items under this after enabling every feature are external authentication settings.
There was a problem hiding this comment.
Authentication would shorter with same meaning. Also if we ever need to add authentication settings related to local authentication that would go under an authentication otherwise we could end up with Authentication and External Authentication.
There was a problem hiding this comment.
Authentication is also about local authentication, the settings of which (related to login and registration) are not under this menu but a level up.
And yes, we could totally have a differentiation here, with a Local Authentication and External Authentication menu (or just one Authentication, but then having all authentication-related things under it).
Additionally, now we have Access Control, Security, and Authentication, which is confusing.
There was a problem hiding this comment.
Access Control is for managing access. Security and Authentication items under the settings menu are are different things.
Security are security related settings for the app itself like header, CORS, https..
If you think we should call it External Authentication, then maybe we should also move the local authentication under "Authentication" so we have two sub menus.
There was a problem hiding this comment.
Looking more at the current setup, I think Registration, User Login, and Change Email are great under "Security". External Authentication providers are under "Authentication" make sense to me without the word "External". If you want to suggest a new setup, please share a bullet bases structure so the picture becomes clear. The following looks great to me.
There was a problem hiding this comment.
I also think that we don't need to have an extra "external" authentication. External vs. local is still all "Authentication" and easy to find there.
There was a problem hiding this comment.
The distinction between Access Control, Security, and Authentication is not big enough, that's my problem; they're terms which denote things that are arguably parts of each other. I understand them, and working back from the menu items and if you know what exactly those are, then it makes sense, but not from the other way around. Like, I want to find the configuration for login; why can't I find it under Access Control? Or Security -> Authentication? These menu items should be commonly clear enough that if you roughly know what you're looking for, you can find your way there.
User Login is a synonym for "authentication", but it's not under the menu Authentication. So again, we should either call the current Authentication menu External Authentication, because it only contains items about that, or move Change Email, Registration, Reset Password, User Login under it too.
However, us arguing about these shows a deeper problem, which I've commented on at the root.
|
#17112 (comment) brings up a larger issue: the majority of this structuring is opinionated and not at all clear to multiple people in the same way. I'm afraid we'll go circles here, and the end result won't make more people happy than where we started. |
Everything that we have today about the UI is opinionated. I am not sure there is a new issue. |
|
The new issue is that the current menu is already out there, and has been for a long time, while this PR moves things around, forcing everyone, developers and users alike, to adapt, and in a significant way. Our conversations in the various comment threads and otherwise show that this is not at all a clear matter, and thus we'll change the current opinion with a different opinion, thinking that it'll be better, but ultimately not having much to back it. UX is not a hard science, but it can still be more methodical than developers sharing their opinions. And in such a case, the status quo wins, because it doesn't need everyone to put work into it. I don't think we should change the menu after conversing just among ourselves. I know, and commend, that you've asked for wider feedback, but us not getting any doesn't mean that most people will be happy with the result. I see no point in arguing among ourselves further about things like whether something is a tool or a setting. Perhaps we're the worst people for this, actually, since it has been very long since we needed to find something in the menu without having a clue, or without understanding the fundamentals. So, taking a step back, I propose rethinking the admin experience, something that's not a new idea by far: with the help of a UX professional (I have ideas on how to find them and Lombiq can fund their work), do the following:
BTW @BenedekFarkas re #11942 (comment): I thought about menu item favoriting too. It's perhaps something that an existing template can offer, but if not, it's not a big deal to implement. |
|
@Piedone At this point, don't you think restarting the process might be too late, given the significant time we've already spent discussing it across various platforms? If you have the time to consult a UI expert to guide the menu structure, I'm all for it. However, I also want to avoid letting this PR sit for too long, especially considering the amount of time and effort already invested by everyone, including yourself. The current structure in this PR reflects the efforts to incorporate all the feedback we've received, aiming to create a solution that works for the majority. |
|
Despite the time spent, no, at this point I wouldn't merge this: the changes are disruptive, so we better be sure of it, lest we have to repeat it. |
Its not. We already provide a backward compatible (legacy mode).
Is there a timeline on when you'll have a UX expert look at this and provide us feedback? |
|
It's disruptive because while you can switch back temporarily, we surely won't maintain two menus forever. At that point, it'll still come at once, it's not something gradual, just optionally delayed. I don't see much point in moving around the menu in itself, because we're missing the forest from the trees: What we need is a path to improve the admin experience, coherently and with guiding principles, not small pieces of it in isolation and without an overarching plan. Nobody will be able to help us with just the menu, without thinking through the whole thing: we might be better off with a different structure of the admin area, to begin with, perhaps with other ways of navigation as well, even secondary as you previously suggested, or whatever else. This will take months, but we'll have something for the long-term. If we want to try this approach, I can look into finding a suitable UX person now, and with the holidays I'd guess start this in earnest mid-January. |
It's temporary if we decide it going to be temporary. It could be a long term or permanent placement.
While you don't, I and many other see the point.
I feel that you are purposely trying to complicating this just because you don't seem to agree with the request. The request is coming from many feedback I personally get. And others, tested it and seem to be happier with it. Anyway, will talk more about this during the next meeting, hopefully you can join. |
|
I also told in the very beginning that:
I'm still suggesting the same. It's not that I merely disagree (I personally actually agree with almost all the changes here, as you've seen); rather, I'm of the viewpoint that these changes, in this form, aren't a net benefit to OC itself and the wider ecosystem. |
I understand. But, unless you are willing to find a consultant to provide us with their opinion, no one will. We already tried to ping the community to get as much feedback as possible. I internally consulted the end users "not developers" as well. So we have been following your suggestion all along and the current state of the PR seems to be accepted by folks other than me like @sebastienros @gvkries and other community members and internal end users. |
|
I am willing, that's what I told above, and Lombiq will fund it. What was the feedback those internal end users had to share about the current menu? |
Can't wait to hear what feedback we get back. Please update us on the process. Hopefully it won't be long time before your consultant some back with some feedback.
They love the current state. They said the menu is much easier to navigate and find what they are looking for. I incorporated the feedback previously which was mainly around settings. |
Fix #11942
Legacy Mode
If you are not ready yet to update the menu in your project, you can enable the legacy format by adding this code to your startup project
Consolidate All Settings Under 'Settings' Menu and Replace 'Configuration' with 'Tools'
Moved the "Content definitions" menu to "Design" tab
Access Control
Settings Menu (All Features Enabled)
Tools Menu
I also moved the Workflows and Audit Trail under Tools instead of the root level.