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

Proposal: add ThemePlugin function to access multilingual theme options #10616

Closed
NateWr opened this issue Nov 15, 2024 · 9 comments
Closed

Proposal: add ThemePlugin function to access multilingual theme options #10616

NateWr opened this issue Nov 15, 2024 · 9 comments
Milestone

Comments

@NateWr
Copy link
Contributor

NateWr commented Nov 15, 2024

Describe the problem
It's cumbersome to access a multilingual theme option in a template. We have to assign it to a variable and then use the locale key to access it.

{assign var="titles" value=$activeTheme->getOption('title')}
{if $titles.$currentLocale}
  {$titles.$currentLocale}
{/if}

Describe the solution
Add a ThemePlugin helper function get a localized option value.

{if $activeTheme->getLocalizedOption('title')}
  {$activeTheme->getLocalizedOption('title')
{/if}

What application are you using?
I'm proposing to add this to OJS 3.3.

Additional information
I appreciate this is a very minor improvement. But it would align ThemePlugin::getOption() with similar data access objects. Not everyone knows that $currentLocale is available in templates.

@NateWr
Copy link
Contributor Author

NateWr commented Nov 15, 2024

Happy to forward port this if you like. But I can work around it if you don't want to add things like this to the LTS.

PRs:
#10617 (stable-3_3_0)
#10652 (stable-3_4_0)
#10654 (main)

Tests:
pkp/ojs#4524 (stable-3_3_0)
pkp/ojs#4538 (stable-3_4_0)
pkp/ojs#4539 (main)

@asmecher
Copy link
Member

@Godoy0722, I think you've run in to a need for this, if I remember right? Could you have a look?

@Godoy0722
Copy link
Contributor

Hello everyone.

I had to run into a need for this on a private client about two months ago. I used another approach on that time, which was to use

{$array = json_decode($activeTheme->getOption("multilingualThemeOption"), true)}

{$localizedCustomBlockButtonLael = $array[$currentLocale]}

I checked out the commit, and it seems to work great. Just wondering if there's a way to make sure the theme option comes back even when it's not a multilingual option - right now it's just returning null in those cases. Is that supposed to happen?

@NateWr
Copy link
Contributor Author

NateWr commented Nov 26, 2024

Generally speaking I think a piece of data is either multilingual or not. I could add a fallback to the primary language, so that it works the same as DataObject::getLocalizedData(). @asmecher what do you think?

@asmecher
Copy link
Member

@NateWr and @Godoy0722, I like the idea of a helper function patterned after DataObject::getLocalizedData. That function does look for a fallback. For 3.3 and 3.4, probably best to keep it simple and duplicate the relevant code in DataObject; for main, it might be worth considering a trait or something.

@NateWr
Copy link
Contributor Author

NateWr commented Dec 4, 2024

@asmecher and @Godoy0722 sorry for the delay. I've now added PRs for stable-3_3_0, stable-3_4_0 and main. They include:

  • ThemePlugin::getLocalizedOption() mimics DataObject::getLocalizedData() in each branch.
  • In main, it introduces a LocalizedData trait. Because ThemePlugin and DataObject store their data differently, it introduces a protected method, getBestLocalizedData(), which is used by ThemePlugin::getLocalizedOption() and DataObject::getLocalizedData().

This was a little more refactoring than anticipated for a simple helper method. Let me know what you think.

NateWr added a commit to NateWr/pkp-lib that referenced this issue Dec 4, 2024
@asmecher
Copy link
Member

asmecher commented Dec 4, 2024

This was a little more refactoring than anticipated...

Not the first or last time I'll hear it! Thanks for your continued good citizenship on this.

The PRs all look good to me (and thanks for the split in approach between stable and dev branches). @Godoy0722, could you have a review as well? With your blessing I'll get these included for the next builds.

@Godoy0722
Copy link
Contributor

It looks good for me as well, @asmecher! Thank you so much, @NateWr, for your time dedicated to it. Since I started here, I have had a good number of themes that asked for multilingual options, and this will definitely save development time for all these themes!

asmecher pushed a commit that referenced this issue Dec 5, 2024
* #10616 Add ThemePlugin function to get localized theme option value

* #10616 Improve method description in LocalizedData trait
@asmecher asmecher added this to the 3.3.0-21 milestone Dec 5, 2024
@asmecher
Copy link
Member

asmecher commented Dec 5, 2024

Merged -- thanks, all!

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

No branches or pull requests

3 participants