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

Move all module settings to Configuration > Settings #11937

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Jul 2, 2022

Fix #11942
Fix #11773
Closes #11972

Move all settings menu items into to one central place (i.e., Configuration > Settings)

Also, make the spacing/formatting consistent in all of the AdminManu classes for consistency and a bit more better readability.

Here is how the menu looks when all the feature the provide settings are enabled

image

Copy link
Member Author

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

I have some question. I would appreciate any feedback to see if more cleaning is needed or learn something new.

@@ -18,17 +18,28 @@ public AdminMenuSignin(IStringLocalizer<AdminMenuSignin> localizer)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why this class and few other are decorated with [Feature] attribute (i.e., [Feature(TwitterConstants.Features.Signin)]?) Each of these classes get registered only when the corresponding feature is enabled (in this case "OrchardCore.Twitter.Signin"). I think adding the [Feature] attribute here is redundant. [Feature] attribute is helpful in some scenarios like a controller, but not sure how it is helpful here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Question for @jtkech
I think these are redundant too. Though, maybe there is a reason.

@hishamco
Copy link
Member

hishamco commented Jul 2, 2022

Please don't request the review from many unless there's a reason for it, regarding the PR I forgot to push my PR regarding the issue I created, but it's already on my local. For the new settings changes that you made let us see if it's fine to move them into configuration menu

Last thing please don't change anything else unrelated to PR, I appreciate the effort you did, bu it's good to focus on fixing the issue itself

Thanks

@Skrypt
Copy link
Contributor

Skrypt commented Jul 2, 2022

@hishamco The request for review to multiple people was automatic as Github added them as "code owners".

@hishamco
Copy link
Member

hishamco commented Jul 2, 2022

I see, but I think you can't remove them before push the PR

@hishamco
Copy link
Member

hishamco commented Jul 8, 2022

@CrestApps I will push a PR for #11773 and merge it then, I will keep this open because what I saw there's a breaking changes in the translation, so please fix the conflict one I merge my PR

Thanks

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.

I'm on half of the reviewing the changes ;) just few notes, please remove unrelated changes to this PR. Fix formating and remove extra whitespaces

@MikeAlhayek
Copy link
Member Author

I'm on half of the reviewing the changes ;) just few notes, please remove unrelated changes to this PR. Fix formating and remove extra whitespaces

I am going to await the outcome of the outcome of the conversation in the issue #11942. I we'll make the necessary changes then.
On a side note, some of the "unrelated" changes you suggested is just general clean up just to unify the code in all of the AdminMenu classes.

@Skrypt
Copy link
Contributor

Skrypt commented Jul 21, 2022

image

@MikeAlhayek
Copy link
Member Author

@sebastienros maybe this is something we should discuss on Tuesday to either close it or approve the change... or request a vote on.

@sebastienros
Copy link
Member

Please vote on the first issue with 👍 and 👎 to decide if we merge or close this PR.

@agriffard
Copy link
Member

Current menus with settings:
image
image
image
image

@urbanit
Copy link
Contributor

urbanit commented Jan 10, 2023

tbh, I expect search settings to be under "Search Module", Security Settings to be under Security, Localization Settings under Localization module etc...
This way custom modules will "keep" their settings inside module area and would be easier for user to locate them
What is proposed, I am afraid it will duplicate the modules structure under "configuration">"settings"

@sebastienros
Copy link
Member

One down vote, nobody else cares.

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