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

Make site title configurable via the theme hook. #11433

Merged
merged 2 commits into from
Oct 22, 2023

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Oct 19, 2023

Summary

  • Allows a theme hook to specify a siteTitle to allow it be configured
  • Creates a core template tag that uses this value or defaults to _("Kolibri")
  • Updates usage of Kolibri in the base.html, unsupported_browser.html and pwa manifest.json to use this template tag

References

Fixes #10958

Reviewer guidance

This only affects the backend usages of the site title, and only standalone usages of Kolibri - a follow up issue will be filed to complete this work, but it would require string changes in order to accommodate fully.

To test this, probably the easiest thing is to go to http://127.0.0.1:8000/ar/unsupported/ and ensure that the page title still displays as كوليبري


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Oct 19, 2023
@rtibbles rtibbles requested a review from marcellamaki October 19, 2023 16:18
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Oct 19, 2023
Return the text of the site title, if provided by the theme. If not, the
default will be returned.
"""
return ThemeHook.get_theme().get("siteTitle", _("Kolibri"))
Copy link
Contributor

Choose a reason for hiding this comment

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

My translation knowledge is shaky at best, but does this support translation of the title in the theme?

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand things, yes, the theme should support translation. The theme is just a Django plugin with a hook which returns a dict of values. It should be able to call _() on the value it returns for siteTitle.

(That would mean we need to set up gettext support for kolibri-endless-key-theme, but that’s fairly straightforward.)

Copy link
Contributor

@pwithnall pwithnall left a comment

Choose a reason for hiding this comment

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

This approach LGTM, thanks very much for putting it together @rtibbles 🥳

Return the text of the site title, if provided by the theme. If not, the
default will be returned.
"""
return ThemeHook.get_theme().get("siteTitle", _("Kolibri"))
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand things, yes, the theme should support translation. The theme is just a Django plugin with a hook which returns a dict of values. It should be able to call _() on the value it returns for siteTitle.

(That would mean we need to set up gettext support for kolibri-endless-key-theme, but that’s fairly straightforward.)

kolibri/core/templatetags/core_tags.py Outdated Show resolved Hide resolved
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

@rtibbles and I discussed the pros and cons of various approaches, and this seems like the option to go with (at least for now!)

@rtibbles rtibbles merged commit d706612 into learningequality:release-v0.16.x Oct 22, 2023
34 checks passed
@rtibbles rtibbles deleted the site_title branch October 22, 2023 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow site title to be customised
4 participants