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

Added /Admin relative URL for admin menu nodes #12807

Merged
merged 9 commits into from
Nov 26, 2022
Merged

Added /Admin relative URL for admin menu nodes #12807

merged 9 commits into from
Nov 26, 2022

Conversation

vengi83644
Copy link
Contributor

This is a change w.r.t #11189

@vengi83644
Copy link
Contributor Author

@Skrypt @hishamco @sebastienros

Hi Team, Please check if this can be moved forward.

Thanks,
R. Venkatesan

@hishamco
Copy link
Member

Thanks for contribution, first of all please close your PR https://github.com/venbacodes/OrchardCore/pull/1 it's not necessary to submit a PR from your repo, then I will try to have a quick look to your changes

@vengi83644
Copy link
Contributor Author

@hishamco I just updated the source code to take the prefix from AdminOptions

@hishamco
Copy link
Member

No need to ping me again, I'm already get a notification when something updated here ;)

Copy link
Member

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

Just fix a little formatting and it's good for me

@vengi83644
Copy link
Contributor Author

Thanks @hishamco , @jtkech.

Should I forward this to someone else for merging?

@hishamco hishamco merged commit b9c2d51 into OrchardCMS:main Nov 26, 2022
@hishamco
Copy link
Member

Thanks for your contribution @vengi83644

@Piedone
Copy link
Member

Piedone commented Jun 16, 2023

Contrary to the PR's title, this adds /Admin to all menu nodes (built via NavigationManager) that use a relative virtual path (e.g. "~/something"). This effectively makes NavigationManager admin-only. I don't think this was the intention, right?

@jtkech
Copy link
Member

jtkech commented Jul 1, 2023

@Piedone Oups that's sad, at some point I thought about being able to use a kind of token {admin} so that if it is present it is replaced by the current admin prefix, otherwise it works as before.

If you think that would be a good compromise, you could open an issue that we could fix.

@Piedone
Copy link
Member

Piedone commented Jul 2, 2023

I think NavigationManager shouldn't be changed, i.e. reverted back. Rather, OrchardCore.AdminMenu should handle what needs to be done to auto-prefix only those menu items that were created via itself. Actually, only LinkAdminNodeNavigationBuilder needs to be affected.

@jtkech
Copy link
Member

jtkech commented Jul 4, 2023

Good catch! Looks like you are right, I may suggest a PR if I have time.

@jtkech
Copy link
Member

jtkech commented Jul 4, 2023

I started something, #13943 and #13944

@Piedone
Copy link
Member

Piedone commented Jul 4, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants