Skip to content
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

Auto add Admin prefix only for Admin nodes. #13944

Merged
merged 11 commits into from
Jul 12, 2023
Merged

Auto add Admin prefix only for Admin nodes. #13944

merged 11 commits into from
Jul 12, 2023

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Jul 4, 2023

Fixes #13943

We recently updated the NavigationManager so that admin menu items are auto prefixed by the AdminUrlPrefix option if not yet done, but as stated by @Piedone #12807 (comment), currently the auto admin prefixing should only be done for menu items built by the LinkAdminNodeNavigationBuilder.

This to not break people using the NavigationManager with other menu nodes but not for admin menus.

@Skrypt
Copy link
Contributor

Skrypt commented Jul 4, 2023

@hishamco NavigationManager is used elsewhere than just the admin UI. I use it to build frontend dynamic menus.

@Piedone
Copy link
Member

Piedone commented Jul 4, 2023

We do that too with Lombiq Base Theme.

@hishamco
Copy link
Member

hishamco commented Jul 6, 2023

@hishamco NavigationManager is used elsewhere than just the admin UI. I use it to build frontend dynamic menus.

Did I ask a question or write a comment about this?

@Skrypt
Copy link
Contributor

Skrypt commented Jul 6, 2023

You just did ask a question 😄
Not sure what you mean there.

@hishamco
Copy link
Member

hishamco commented Jul 6, 2023

I was OFF for a week, I was surprised when you mentioned me while I didn't ask a question or write a comment, maybe I asked somewhere else

@@ -25,7 +25,7 @@ public class ListsAdminNodeNavigationBuilder : IAdminNodeNavigationBuilder
private ListsAdminNode _node;
private ContentTypeDefinition _contentType;

private const int MaxItemsInNode = 100; // security check
private const int _maxItemsInNode = 100; // security check.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine for constants

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, but Visual Studio shows a warning, because of the new rule we have on private fields (leading underscore required) in the editor config, maybe there is a way to exclude private const from this rule. Note: We also have warnings on private localizer fields as S and H.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I reverted the renaming of this const

I suggested here #13953 to ignore the underscore rule for private const.

@jtkech jtkech requested a review from Piedone July 7, 2023 22:42
@jtkech jtkech requested a review from agriffard as a code owner July 12, 2023 19:40
@jtkech jtkech merged commit ee973c1 into main Jul 12, 2023
@jtkech jtkech deleted the jtkech/admin-menu branch July 12, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto add Admin prefix only for Admin nodes.
5 participants