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

UX: Add THEME_SITENAME as a suffix in <head> #382

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

UX: Add THEME_SITENAME as a suffix in <head> #382

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cc8614
UX: Add THEME_SITENAME as a suffix in <head>
carlinmack Mar 19, 2024
e29f697
bug: prevent title | title
carlinmack Apr 5, 2024
7931d03
refactor: shorten logic
carlinmack Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions invenio_theme/templates/semantic-ui/invenio_theme/page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@
{%- endblock head_meta %}

{%- block head_title %}
{%- set title = title or _(config.THEME_SITENAME) or _('Invenio') %}
<title>{{title}}</title>
<title>{%- if title %}{{title}} | {% endif -%}{{ _(config.THEME_SITENAME) or _('Invenio') }}</title>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM code-wise.
all the instances running invenio will be affected by this change, was that the goal ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is beneficial for all instances in my opinion

The only problem I can foresee is if they haven't set THEME_SITENAME then they will have Invenio appended

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a section "Update configuration variables" under
https://github.com/inveniosoftware/docs-invenio-rdm/pull/607/files#diff-39da3e2af9ef047f00739348879d5a91714f7b1cf3f693c959a9e6154c2b9795R125
explaining exactly this? It should go into the upgrade guidelines for v12

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment! btw this variable has already been contributed to cookiecutter :) inveniosoftware/cookiecutter-invenio-rdm#273

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to merge from me if there is not a chance we'll have Zenodo | Zenodo? For that we'll just need to check if title is every set to config.THEME_SITENAME

{%- endblock head_title %}

{%- block head_links %}
Expand Down
Loading