Issue 29622568: Issue 6013 - Add site name to document title in website-defaults standard metadata (Closed)
|
Created:
Nov. 28, 2017, 9:11 a.m. by ire Modified:
Dec. 4, 2017, 11 a.m. Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionIssue 6013 - Add site name to document title in website-defaults standard metadata
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use config.has_option, use title_exclude_sitename #
Total comments: 2
Patch Set 3 : Rephrase if statement #
Total comments: 2
Patch Set 4 : Use get_string to get site name #
Total comments: 4
MessagesTotal messages: 16
https://codereview.adblockplus.org/29622568/diff/29622569/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29622568/diff/29622569/includes/meta/stand... includes/meta/standard.tmpl:5: {% if page == "index" or config.get("general", "sitename") is not defined %} This currently doesn't work because if I try "config.get("general", "sitename")" where there is no sitename, I get a 500 error. I can't seem to figure out how to check if `config.get("general", "sitename")` exists without breaking things. Can you help?
https://codereview.adblockplus.org/29622568/diff/29622569/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29622568/diff/29622569/includes/meta/stand... includes/meta/standard.tmpl:5: {% if page == "index" or config.get("general", "sitename") is not defined %} I don't think that the index page will be the only page that we want to exclude the sitename from. I suggest that we use an optional page attribute instead e.g. {% if title_exclude_sitename or ... https://codereview.adblockplus.org/29622568/diff/29622569/includes/meta/stand... includes/meta/standard.tmpl:5: {% if page == "index" or config.get("general", "sitename") is not defined %} On 2017/11/28 09:12:49, ire wrote: > This currently doesn't work because if I try "config.get("general", "sitename")" > where there is no sitename, I get a 500 error. I can't seem to figure out how to > check if `config.get("general", "sitename")` exists without breaking things. Can > you help? I think that this is how to find the answer to your question (connect the dots): - Use grep (or similar) to find "config" in the "cms" repository - https://hg.adblockplus.org/cms/file/tip/cms/sources.py#l44 - https://hg.adblockplus.org/cms/file/tip/cms/sources.py#l70 - https://hg.adblockplus.org/cms/file/tip/cms/sources.py#l18 - https://docs.python.org/2/library/configparser.html - https://docs.python.org/2/library/configparser.html#ConfigParser.SafeConfigPa... - https://docs.python.org/2/library/configparser.html#configparser-objects - https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigParser - https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigPar...
https://codereview.adblockplus.org/29622568/diff/29622569/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29622568/diff/29622569/includes/meta/stand... includes/meta/standard.tmpl:5: {% if page == "index" or config.get("general", "sitename") is not defined %} On 2017/11/28 14:49:21, juliandoucette wrote: > I don't think that the index page will be the only page that we want to exclude > the sitename from. I suggest that we use an optional page attribute instead e.g. > {% if title_exclude_sitename or ... Acknowledged. https://codereview.adblockplus.org/29622568/diff/29622569/includes/meta/stand... includes/meta/standard.tmpl:5: {% if page == "index" or config.get("general", "sitename") is not defined %} On 2017/11/28 14:49:21, juliandoucette wrote: > On 2017/11/28 09:12:49, ire wrote: > > This currently doesn't work because if I try "config.get("general", > "sitename")" > > where there is no sitename, I get a 500 error. I can't seem to figure out how > to > > check if `config.get("general", "sitename")` exists without breaking things. > Can > > you help? > > I think that this is how to find the answer to your question (connect the dots): > > - Use grep (or similar) to find "config" in the "cms" repository > - https://hg.adblockplus.org/cms/file/tip/cms/sources.py#l44 > - https://hg.adblockplus.org/cms/file/tip/cms/sources.py#l70 > - https://hg.adblockplus.org/cms/file/tip/cms/sources.py#l18 > - https://docs.python.org/2/library/configparser.html > - > https://docs.python.org/2/library/configparser.html#ConfigParser.SafeConfigPa... > - https://docs.python.org/2/library/configparser.html#configparser-objects > - > https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigParser > - > https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigPar... Alright, thanks!
New patch set uploaded
CC Vasily && Jon In this PatchSet Ire is trying to add a website name to each page's document.title. In order to do that, she defined a sitename property in settings.ini and output it inside the standard meta data include's title tag. The problem is that she didn't translate sitename. I think that we may want to translate sitename in some cases/languages. But, if we use the translate filter, then the website name will be included in each pages JSON file. Instead, we often use the `get_string` global provided by the CMS and manually create a locale file to "get the string" from. I don't like the way that we do this because we have to manually create a locale file that is not associated with a page (like the others are), define the string there, and refer to it by name in the template (and I think that this can be confusing if one don't already know how it works). What do you think? (We may use get_string now, but I wanted to ask anyway.)
On 2017/11/29 20:46:17, juliandoucette wrote: > CC Vasily && Jon > > In this PatchSet Ire is trying to add a website name to each page's > document.title. In order to do that, she defined a sitename property in > settings.ini and output it inside the standard meta data include's title tag. > The problem is that she didn't translate sitename. I think that we may want to > translate sitename in some cases/languages. But, if we use the translate filter, > then the website name will be included in each pages JSON file. Instead, we > often use the `get_string` global provided by the CMS and manually create a > locale file to "get the string" from. I don't like the way that we do this > because we have to manually create a locale file that is not associated with a > page (like the others are), define the string there, and refer to it by name in > the template (and I think that this can be confusing if one don't already know > how it works). > > What do you think? > > (We may use get_string now, but I wanted to ask anyway.) Hey, Yup, I would agree, that seems a bit heavy handed. Perhaps Vasily and I can look at streamlining this. What do you think Vasily?
On 2017/11/29 20:46:17, juliandoucette wrote: > CC Vasily && Jon > > In this PatchSet Ire is trying to add a website name to each page's > document.title. In order to do that, she defined a sitename property in > settings.ini and output it inside the standard meta data include's title tag. > The problem is that she didn't translate sitename. I think that we may want to > translate sitename in some cases/languages. But, if we use the translate filter, > then the website name will be included in each pages JSON file. Instead, we > often use the `get_string` global provided by the CMS and manually create a > locale file to "get the string" from. I don't like the way that we do this > because we have to manually create a locale file that is not associated with a > page (like the others are), define the string there, and refer to it by name in > the template (and I think that this can be confusing if one don't already know > how it works). > > What do you think? > > (We may use get_string now, but I wanted to ask anyway.) The approach with `get_string` and a separate locale file seems reasonable, and should not be too confusing if one knows what `get_string` does. We could use on of the existing locale files to avoid creating locale files not attached to pages, but it seems like there's no way to avoid having some locale file if we want to translate the sitename. If there are multiple variables in `settings.ini`, we could think about a system-wide approach for translating config variables via a specialized locale file, but for one variable such mechanism seems like a bit of an overkill. All in all, I don't immediately see a more elegant solution than the one you've described, but I'm open to suggestions.
Hey Ire, A small nit about the template logic. Cheers, Vasily https://codereview.adblockplus.org/29622568/diff/29622610/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29622568/diff/29622610/includes/meta/stand... includes/meta/standard.tmpl:5: {% if title_exclude_sitename or config.has_option("general", "sitename") == False %} The second part of this condition can be written as `or not config.has_option(...)` -- perhaps a bit more readable this way.
On 2017/11/30 12:53:29, Vasily Kuznetsov wrote: > Hey Ire, > > A small nit about the template logic. > > Cheers, > Vasily > > https://codereview.adblockplus.org/29622568/diff/29622610/includes/meta/stand... > File includes/meta/standard.tmpl (right): > > https://codereview.adblockplus.org/29622568/diff/29622610/includes/meta/stand... > includes/meta/standard.tmpl:5: {% if title_exclude_sitename or > config.has_option("general", "sitename") == False %} > The second part of this condition can be written as `or not > config.has_option(...)` -- perhaps a bit more readable this way. Thanks. I took your suggestion https://codereview.adblockplus.org/29622568/diff/29622610/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29622568/diff/29622610/includes/meta/stand... includes/meta/standard.tmpl:5: {% if title_exclude_sitename or config.has_option("general", "sitename") == False %} On 2017/11/30 12:53:28, Vasily Kuznetsov wrote: > The second part of this condition can be written as `or not > config.has_option(...)` -- perhaps a bit more readable this way. I like your suggestion better. Done.
Thank you for your feedback Jon & Vasily. Can I rely on you to create a ticket if necessary? https://codereview.adblockplus.org/29622568/diff/29625604/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29622568/diff/29625604/includes/meta/stand... includes/meta/standard.tmpl:8: <title>{{ title | translate("page-title", "Page title") }} | {{ config.get("general", "sitename") }}</title> As mentioned before, sitename is not currently translated. If you want to translate it site wide then you have to use get_string and create a locale file.
On 2017/12/01 15:37:44, juliandoucette wrote: > Thank you for your feedback Jon & Vasily. Can I rely on you to create a ticket > if necessary? Sure, although at the moment I still don't see any content for a ticket. The solution with `get_string` looks good to me and unless we decide to add something special handling for sitenames, it seems like no ticket is needed.
https://codereview.adblockplus.org/29622568/diff/29625604/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29622568/diff/29625604/includes/meta/stand... includes/meta/standard.tmpl:8: <title>{{ title | translate("page-title", "Page title") }} | {{ config.get("general", "sitename") }}</title> On 2017/12/01 15:37:44, juliandoucette wrote: > As mentioned before, sitename is not currently translated. If you want to > translate it site wide then you have to use get_string and create a locale file. Done.
LGTM https://codereview.adblockplus.org/29622568/diff/29628710/locales/en/site.json File locales/en/site.json (right): https://codereview.adblockplus.org/29622568/diff/29628710/locales/en/site.jso... locales/en/site.json:3: "message": "eyeo Website Defaults" suggest: Add a message description "website title suffix" https://codereview.adblockplus.org/29622568/diff/29628710/settings.ini File settings.ini (right): https://codereview.adblockplus.org/29622568/diff/29628710/settings.ini#newcode5 settings.ini:5: Did you mean to add a sitename?
https://codereview.adblockplus.org/29622568/diff/29628710/settings.ini File settings.ini (right): https://codereview.adblockplus.org/29622568/diff/29628710/settings.ini#newcode5 settings.ini:5: On 2017/12/04 10:33:08, juliandoucette wrote: > Did you mean to add a sitename? Is that still needed if we're not using it from this config?
https://codereview.adblockplus.org/29622568/diff/29628710/settings.ini File settings.ini (right): https://codereview.adblockplus.org/29622568/diff/29628710/settings.ini#newcode5 settings.ini:5: On 2017/12/04 10:41:09, ire wrote: > On 2017/12/04 10:33:08, juliandoucette wrote: > > Did you mean to add a sitename? > > Is that still needed if we're not using it from this config? No ( oops :D ) 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
