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.

Sign up for GitHub

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

Jump to bottom

Theme related suggestions #1229

Closed
darek01 opened this issue Dec 27, 2016 · 4 comments
Closed

Theme related suggestions #1229

darek01 opened this issue Dec 27, 2016 · 4 comments
Labels
enhancement in progress

Comments

@darek01
Copy link

darek01 commented Dec 27, 2016

Hi, I have some suggestions related to Base Template, theme tutorial and variables, from the theme developer's perspective.

{% set theme_config = attribute(config.themes, config.system.pages.theme) %}
That's not great to be required to define a basic variable by default in a template. I found it's already available in grav as config.theme so maybe we can delete this from Base Template and docs.

<html lang="{{ grav.language.getActive ?: theme_config.default_lang }}"> ->
<html lang="{{ page.language ?: site.language }}">
There's already page.language available, just introduce default variable site.language and it gets a lot cleaner. theme_config.default_lang is not the best because it should be default site language as set by the user and not theme language as set by theme developer. default_lang is also not even defined in the theme config of Base Template.

<title>{% if header.title %}{{ header.title|e('html') }} | {% endif %}{{ site.title|e('html') }}</title> ->
<title>{% if page.title %}{{ page.title|e }} | {% endif %}{{ site.title|e }}</title>
page.title is more intuitive and doesn't introduce new top-level variable, 'html' is default for escape filter

<link rel="canonical" href="{{ page.url(true, true) }}" />
Maybe consider introducing some new variable like page.permalink but for canonical, e.g. page.canonical

{{ page.header.body_classes }} - > {{ page.body_classes }}

{{ base_url == '' ? '/' : base_url }}
Can't base_url just return '/' instead of ''? Also I had to check where it came from as it's a new top-level variable. Something like site.url would be more intuitive.

{{ config.site.title }} -> {{ site.title }}

Generally speaking I'd make most of variables available as x.y and not x.y.z, while also limiting the number of top-level x variables available.

config.system.variable -> system.variable
config.theme.variable -> theme.variable
config.site.variable -> site.variable
page.header.variable -> page.variable
header.variable -> page.variable

I think these changes would make Base Template, documentation, and workflow with grav more clean and intuitive. Some of them are already supperted but not documented as default worflow.
@rhukster
Copy link
Member

rhukster commented Dec 28, 2016 •
edited
Loading

Some of these are good ideas and would definitely make things a little cleaner. As i'm already actually working on a new default theme, i'll see what I can do about adding some of these improvements.

The one thing that is not relaly possible, is the header.variable -> page.variable. There could be conflicts here as the header is a flexible YAML powered set of variables, where as the page itself, is an object that represents the page. The header. is simply a shortcut for page.header.

@darek01
Copy link
Author

darek01 commented Dec 28, 2016

Grav looks good, I'm glad to provide some feedback.

About header.variable -> page.variable, I put {{ page.title }} in a template and it also alredy works as shortcut for {{ page.header.title }}! Is it discouraged to use page.title, and rather type header.title or page.header.title? It's easier to remember and more consistent to me to just use page.language and page.title instead of page.language but headers.title or page.headers.title. Also that way I can have mirrored default variables in site. and just type: {{ page.title|e }} | {{ site.title|e }} or {{ page.language ?: site.language }}.

@rhukster
Copy link
Member

rhukster commented Jan 3, 2017

Quite a few improvements have been taken care of here: #1232

@rhukster
Copy link
Member

rhukster commented Jan 6, 2017

This PR has been merged, just need to document it all!

@rhukster rhukster closed this as completed Jan 6, 2017
vrabe added a commit to vrabe/grav-theme-yocto that referenced this issue Feb 18, 2019
@vrabe
solve the problem: language in html tag does't show
a493ed0 
getgrav/grav#1229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement in progress
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rhukster @darek01