Theme inheritance: hardcoded theme names vs. theme config about grav-theme-knowledge-base HOT 9 CLOSED

Thank you for the well-researched issue report. I, too, am surprised nobody has mentioned this before. I'll release an update later today.

Grav theme "inheritance" isn't really inheritance at all. You simply replace a file. And because the core instructions tell you to create the theme yaml file from scratch, it simply overwrites the base yaml file. It doesn't extend it. So here's what I'll try to get done today:

• Update the templates to use the correct variable.
• Update the theme's README to explicitly mention how to properly inherit it.
• Submit an issue to Grav suggesting a more intuitive way to inherit yaml files.
• Submit some PRs against Grav's documentation to help future theme developers and customizers.

I'll post here again when the updates are done. Thanks again!

from grav-theme-knowledge-base.

The actual best practice is laid out in the docs. So I'm going to drop that line from base.html.twig and change all explicit callouts to the theme by name to grav.theme.config (the longest but most explicit alternate form). It appears to be working in my testing.

It's still not backwards compatible, so I'll have to write up some things, so it won't be done today. They accepted my PR against the inheritance docs, though. I'm making another PR against the theme configuration page to make the best practices explicit.

from grav-theme-knowledge-base.

v2.0.0 released. Thanks!

from grav-theme-knowledge-base.

Thanks for the swift reply! I'm glad my report seems helpful.

Let me add one more thing I just realized now: I had previously not tried how a config in user/config/themes would behave with inheritance, I was only using the theme folder directly. After a quick test, I can now confirm that in an inheritance setup, having a user/config/themes/knowledge-base.yaml leads to config.themes['knowledge-base'] not being null, but instead fetch its values from there. I guess this explains why others didn't have issues - they probably had this file present, as is recommended in your README.

Hence, implementing the suggested changes should not break anything that wasn't already broken. It also suggests that the best way of theme inheritance and configuration is probably as follows:

• Have the theme templates use theme_config as described above
• Use the /user/theme/derived-theme/derived-theme.yaml only for the streams section
• Copy knowledge-base.yaml to /user/config/themes/derived-theme.yaml and do custom configuration there

from grav-theme-knowledge-base.

But after changing things to theme_config, the file user/config/themes/knowledge-base.yaml is no longer seen. Hrm...

from grav-theme-knowledge-base.

I think it's still most correct to go the theme_config route, but it appears the update would not be backwards compatible. So I'm going to need a little time to decide on the best way to approach this.

I'm going to file an issue with Grav and see what the best practice might be here. Ideally theme inheritance would actually inherit the yaml files instead of replacing them. Obviously you can't do that with twig, but it would work fine with yaml.

from grav-theme-knowledge-base.

Thanks for the update.

I am using the Themer plugin (https://github.com/Sommerregen/grav-plugin-themer) and the default theme used isn't Knowledge Base. I was previously using config.themes['derived-theme'] and it worked perfectly. The update broke the theme though and I had to replace grav.theme.config by config.themes.derived-theme for it to work. So all good now.

I guess my question is: is there any way to make it work automatically without tweaking the templates? i.e. to select which theme config file to look up when this isn't the "activated" theme (as defined by Themer plugin) Because it looks like grav.theme.config looks up the activated theme.

The theme works now for me so I am all good, but I thought I would mention it as this is something to consider. Thank you for this great theme (-:

from grav-theme-knowledge-base.

This is an interesting scenario. I hadn't heard of Themer before now. By "broke the theme" do you mean just that the sidebar disappeared? Or did something more dramatic happen?

You are correct that grav.theme.config points to the currently activated theme. But it looks like the Themer plugin is indeed activating the theme and updating the whole Grav config object. Did you copy the data from the config file from themes/knowledge-base into themes/derived-theme? If so, then I would expect Themer to still correctly resolve grav.theme.config. Assuming my pull requests get approved, then you wouldn't need to do this and the merging would happen automatically.

I think the grav.theme.config approach is the best practice for most situations. Someone derives a theme, they want to be able to customize it without changing the templates to manually point to the derived theme's config file.

• I've submitted a PR against the docs to make this (IMO) best practice explicit.
• I've submitted a PR that will make it the default behaviour to merge parent and child config files.

Neither have been reviewed yet, so if Grav doesn't think they're best practices, then I'll have to rethink everything. I'm open to reverting my change to this theme or restructuring it entirely.

from grav-theme-knowledge-base.

I am very sorry it took me so long to reply. I didn't see your answer.

I have copied the data from the config file into the active theme config file and indeed it works (-:

Thanks a lot for this awesome theme (-:

from grav-theme-knowledge-base.