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

Theme inheritance: hardcoded theme names vs. theme config #24

Closed
domsson opened this issue Aug 9, 2018 · 9 comments
Closed

Theme inheritance: hardcoded theme names vs. theme config #24

domsson opened this issue Aug 9, 2018 · 9 comments

Comments

@domsson
Copy link

@domsson domsson commented Aug 9, 2018

When inheriting from the knowledge-base theme, one might wish to change the theme config in the derived theme, without touching the original. And while Grav apparently does not inherit a base theme's config, this can be achieved by copying over the param section of user/themes/knowledge-base/knowledge-base.yaml into the derived theme's config file (underneath or above the streams section responsible for the inheritance).

However, knowledge-base is accessing the theme config via config.themes['knowledge-base'], using the hardcoded theme name. This is null in a derived theme. Only config.themes['derived-theme'] (assuming the name of the child theme to be derived-them) is defined there. This leads to at least two issues:

  • The inherited theme breaks in several places (no sidebar, blacklist not working, ...)
  • Config changes to the inherited theme (performed as outlined above) take no effect

This issue has also been present in the deliver theme, leading to this issue, for example. The last reply there also outlines the solution:

  1. Add the following at the top of the base template:

     {% set theme_config = attribute(config.themes, config.system.pages.theme) %}
    

This is already present in the knowledge-base\templates\partials\base.html.twig, but never used.

  1. Then use theme_config instead of config.themes['theme-name'] in all template files.

theme_config will evaluate to config.themes['knowledge-base'] if the theme is used directly (hence, everything works exactly as before) and to config.themes['derived-theme'] in a derived theme. As per Flavio Copes' reply linked above, this is how it should be done.

Implementing this is as trivial as a search and replace in the template files. However, I'm not sure if this could potentially break existing setups for users? I don't think it should, though. Also, I'm wondering why no other users have reported this issue, while there apparently are people who do inherit from knowledge-base, as evident by this issue report. Shouldn't the sidebar be missing for them, amongst other issues? Maybe I am missing or confusing something here.

Side note: I think this is a bit of an issue with Grav. One would expect that if a specific config setting is not present in the derived theme's config, Grav would go looking in the base theme's config. This is what the tutorial about theme inheritance implies when it says "You can of course customize any part of the original base-theme". However, as per my testing as well as the forum link above, this seems not to be the case, making the whole them inheritance a bit inconsistent / counter-intuitive.

@domsson domsson changed the title Theme inheritance: hardcoded theme names vs. theme config modifications Theme inheritance: hardcoded theme names vs. theme config Aug 9, 2018
@Perlkonig

This comment has been minimized.

Copy link
Owner

@Perlkonig Perlkonig commented Aug 9, 2018

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!

@domsson

This comment has been minimized.

Copy link
Author

@domsson domsson commented Aug 9, 2018

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
@Perlkonig

This comment has been minimized.

Copy link
Owner

@Perlkonig Perlkonig commented Aug 9, 2018

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

@Perlkonig

This comment has been minimized.

Copy link
Owner

@Perlkonig Perlkonig commented Aug 9, 2018

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.

@Perlkonig

This comment has been minimized.

Copy link
Owner

@Perlkonig Perlkonig commented Aug 9, 2018

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.

@Perlkonig

This comment has been minimized.

Copy link
Owner

@Perlkonig Perlkonig commented Aug 9, 2018

v2.0.0 released. Thanks!

@Perlkonig Perlkonig closed this Aug 9, 2018
@happywebio

This comment has been minimized.

Copy link

@happywebio happywebio commented Aug 17, 2018

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 (-:

@Perlkonig

This comment has been minimized.

Copy link
Owner

@Perlkonig Perlkonig commented Aug 17, 2018

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.

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.

@happywebio

This comment has been minimized.

Copy link

@happywebio happywebio commented Dec 31, 2018

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 (-:

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.