Add support for theme settings

[Osama]

Continuing the discussion from Discourse Tab Bar for Mobile:

One feature that Discourse doesn’t currently offer is the ability for themes to have some sort of settings or variables that would work similar to how site/plugin settings work i.e. theme settings would be defined with default values by theme developers, and then site owners could override those default values and they’d still be able to get new updates for the theme without worrying about losing their configurations.

How should this work?

Obviously this is not complete specifications - this is just a rough idea on how I see this working. I’m keen to hear your ideas and thoughts

a) For site owners

I think all we need to do here is add a simple button somewhere on the theme page. that navigates to a new route /admin/customize/themes/:theme_id/settings and the new route will display the settings which will look like this:

b) For theme developers

I think a new field next to CSS and HTML fields that lets theme developers type their theme settings in YAML (or JSON?) format should do the job. Mockup:

(Note “help” is the text that will be displayed below the setting that explains what the setting does)

c) Supported data types

I think we should have setting types that support these data types:

Integer
String
Boolean
Enum
List

d) Accessing theme settings in theme JS code

How will theme developers access theme settings in their JS code? I’m thinking something like Discourse.ThemeSettings.theme_setting_01 but this won’t help with naming collision (when two themes have a setting with the same name) which I’m wondering how should be handled?

For now that’s all I have. Happy to hear how you see this working

cc @sam

[sam (Sam Saffron)]

I like this but think the vars should be done more like how they are done in the “uploads” section.

Then settings can just show up inline in the front screen.

[joffreyjaffeux]

Just want to add that you probably want to support localisation here.

[Osama]

Localisation for the settings help text right? Should we have another field dedicated for translations, or should translations go in the settings field like this ?

setting_01:
    help:
        en: "foo bar"
        fr: "foo bar french"

[Osama]

PR for this is up:

https://github.com/discourse/discourse/pull/5562

It works pretty much exactly like the OP describes except for one little thing: I created a separate “Setting” tab for the YAML field instead of nesting it under the Common tab. To me it makes for sense to have its own tab. Open to revert it back if you like the proposal in the OP.

Screenshot with all supported types:

Screenshot when there are no settings (should we omit the settings section?):

[sam (Sam Saffron)]

This is now merged in!

I also implemented the ability consume settings in theme JavaScript like so: https://github.com/discourse/discourse-linkify-words/blob/master/common/head_tag.html#L3 (must be a type="test/discourse-plugin")

@Osama I feel we do need a few refinements if you feel up to it:

Not a huge fan of having this in the UI, I am fine for all the config to happen in the remote git repo. This is very much a feature for ninja theme creators and not the general public.

We don’t need this, cause we already have a theme section:

Lastly, we need a fancy howto here explaining how to use it and to update:

Structure of themes and theme components

Discourse supports native themes that can be sourced from a .tar.gz archive or from a remote git repository including private repositories. [56] An example theme is at: GitHub - discourse/discourse-simple-theme: Sam's simple discourse theme [32] The git repository will be checked for updates (once a day), or by using the Check for Updates button. When changes are detected the Check for Updates button will change to the Update to Latest. [image] To create a theme you need to follow a spe…

If you can give it a shot

[Osama]

Here is a PR for the refinements: UX: Improvements for theme UI by OsamaSayegh · Pull Request #5650 · discourse/discourse · GitHub

@sam

Lastly, we need a fancy howto here explaining how to use it and to update:

I have this on my TODO post-merge list, I’m working on it I’ll try to get it out it by this weekend.

[Osama]

@sam Is there a reason why the local settings JS variable only includes settings that are explicitly used like this settings.some_setting_name and doesn’t just include all settings of the theme?

https://github.com/discourse/discourse/blob/9331b4849dbcec34057fa76a4cf0f5d98346b155/app/models/theme_field.rb#L32

To me this looks like it’s not going to cover advanced use-cases where a theme dev would want to do something like this:

settings[`first_${some_variable}_third`]

but wouldn’t be able to.

[sam (Sam Saffron)]

It feels a bit over the top, I did this to cut down on potential bloat if there are lots of settings.

Not against tweaking this, but I got to see a real use case.

[Osama]

I’ll admit I don’t have a real use-case, I was testing it and noticed that I had to that to be able use my setting. Yeah let’s wait and see if anyone will run into a problem because of this.

[tshenry (Taylor)]

Is there by chance a way to access theme settings in a script of type="text/x-handlebars"?

I’ve been able to use settings.name to get the value in a script of type="text/discourse-plugin" (and it’s awesome I must say!), just wondering if it’s possible to get a setting value in a handlebars script somehow?

I’m still getting my bearings with all of this, so forgive me if there’s something simple I’m missing to handle this

[sam (Sam Saffron)]

Oh yeah … PR completely welcome on this. Just need to figure out how to shuffle the data there. (we can’t do a search replace here, we would have to pass in the setting objects to the context)

[Osama]

@sam

Oh yeah … PR completely welcome on this. Just need to figure out how to shuffle the data there. (we can’t do a search replace here, we would have to pass in the setting objects to the context)

@sam I’m looking into this right now and my plan to go about this is to create a little handlebars helper that would take a key and value as params and inject them into the template context. Then on the server side we get all text/x-handlebars scripts and before they’re compiled, we prepend to them something this like for each theme setting:

{{settings-helper context=this key="setting_name" value=setting_value}}

Does that sound good to you? Do you think there is a better way to go about this?

[Osama]

PR created:

https://github.com/discourse/discourse/pull/5731

With this change, theme devs will be able to use theme settings in handlebars scripts like so:

<script type='text/x-handlebars' data-template-name='/connectors/discovery-list-container-top/my-template'>
    <div class="test-div">
      {{#if themeSettings.boolean_setting}}
        {{themeSettings.string_setting}}
      {{/if}}
    </div>
</script>

[vinothkannans (Vinoth Kannan)]

@Osama

With this change, theme devs will be able to use theme settings in handlebars

It’s not working properly in raw handlebar templates. Here incorrect context variable hash.data.root is used instead of hash.hash.context. But even fixing it not resolving the problem since the context itself carrying incorrect value.

In this line instead to set this object to context variable the raw handlebar precompiler setting constant string value "this" to the context (context = "this"). It is working perfectly in normal handlebars since it using different precompiler.

We have to fix the raw handlebar precompiler to resolve this issue. Until that I did Remove support for theme settings in raw handlebar templates · discourse/discourse@19e8f9a · GitHub

[Osama]

Ah, apologize about that. It seems my implementation worked in some cases but not all because I tested it by overriding the list/topic-list-item.raw template and it worked fine. Could you tell me which raw template my implementation failed to work with so I can debug this further?

[vinothkannans (Vinoth Kannan)]

Yes, after debugging it further I found that not all the raw templates raising the issue. It only happens when a theme include a raw-plugin-outlet connector template. For example when I use the raw plugin outlet “topic-list-before-status” in below theme it breaks the UI.

https://github.com/vinothkannans/discourse-test-theme/blob/38caa5dd68ed8fabd5561545a59d22715f8a355f/common/header.html#L1-L3

So the issue may not be in precompiler. Sorry for my previous misguided reply. But we have to resolve this issue before supporting theme settings for raw templates.

[Osama]

There is actually an issue with the precompiler that it doesn’t recognize this as the template’s context object. However, I just realized that we don’t really need the precompiler to recognize this because the helper function here is bound to the template context object, so this inside the helper function refers to the template context, which is exactly what we need here. Will put together PR to demonstrate what I mean.

EDIT:

PR sent: https://github.com/discourse/discourse/pull/5758 cc @vinothkannans

Tested this change with 4 different scenarios:

• overriding an Ember template
• overriding a raw template
• using a plugin outlet in an Ember template
• using a plugin outlet in a raw template

And it worked in all of them

[vinothkannans (Vinoth Kannan)]

Now it’s working fine, thank you @Osama