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

Make General settings page extensible #14106

Merged
merged 3 commits into from Aug 15, 2023

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Aug 10, 2023

Currently, the General settings page does not allow us to add more settings to it. With this simple change, anyone that wants to add settings to the general settings is able to using drivers. The UI itself did not change.

To extend these views, create a new display driver using "general" as the group id. Then place your custom settings into one of the 3 existing tabs or a new custom tab. Here is an example to inject a custom settings from MyCustomSettings class into a new tab on the general settings called "New Tab"

public class MyCustomSettingsDisplayDriver : SectionDisplayDriver<ISite, MyCustomSettings>
  {
      public override IDisplayResult Async(MyCustomSettings settings)
      {
          Initialize<SiteSettingsViewModel>("MyCustomSettings_Edit", model => {
              //...
          }).Location("Content:1#New Tab;40")
          .OnGroup(DefaultSiteSettingsDisplayDriver.GroupId); // Or "general" if you prefer a string
      }
  }

image

@jtkech
Copy link
Member

jtkech commented Aug 10, 2023

For info we already have the OC.CustomSettings module.

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Aug 10, 2023

yes we do. But there are times when a users want to add ISite settings directly to the general settings. The settings menu is super long and this could help.

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.

We need to change settings views to react to the code suggestion

.Location("Content:1#Site;10")
.OnGroup(GroupId),

Initialize<SiteSettingsViewModel>("SettingsResources_Edit", model => PopulateProperties(site, model))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Initialize<SiteSettingsViewModel>("SettingsResources_Edit", model => PopulateProperties(site, model))
Initialize<SiteSettingsViewModel>("Settings_Resources_Edit", model => PopulateProperties(site, model))

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure what would the file name be if we changed it as suggested. SettingsResources_Edit > SettingsResources.Edit.cshtml.

Copy link
Member

Choose a reason for hiding this comment

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

As I remember _ becomes a dot . and a double underscores __ becomes a dash -.

So Settings__Resources_Edit with double underscores would have been resolved to Settings-Resources.Edit.

But I think that here _Edit is considered as the display type, if so the convention is to place the display type just after the shape base type, this because the display type is discovered as the first element separated with a _, this allows to add other elements separated with a _, then when converted to a file name it is moved to the end.

So Settings_Edit__Resources would have been the right template name and still resolved to Settings-Resources.Edit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks you! I'll try it

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtkech Indeed that worked!

What about the wrapper name? Would we change GeneralSettingsWrapper? How can we generate GeneralSettings.Wrapper.cshtml?

Copy link
Member

Choose a reason for hiding this comment

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

Only from memory so should be tried.

GeneralSettings_Wrapper => GeneralSettings.Wrapper

GeneralSettings_Wrapper__OtherTerm => GeneralSettings-OtherTerm.Wrapper

Hmm, maybe in TheBlogTheme there is a wrong file name Widget.Wrapper-Zone-HeadMeta.liquid, maybe it should be Widget-Zone-HeadMeta.Wrapper.liquid.

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure what that shape is or does. maybe a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, was just for info when I saw it

@sebastienros
Copy link
Member

Demo (discussion) on Tuesday

@MikeAlhayek MikeAlhayek merged commit 222f0e8 into OrchardCMS:main Aug 15, 2023
3 checks passed
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.

None yet

4 participants