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

Hard to implement a ContentCulturePicker in Liquid #16202

Closed
bashuss opened this issue May 31, 2024 · 18 comments · Fixed by #16208
Closed

Hard to implement a ContentCulturePicker in Liquid #16202

bashuss opened this issue May 31, 2024 · 18 comments · Fixed by #16208

Comments

@bashuss
Copy link

bashuss commented May 31, 2024

@sebastienros

ContentCulturePicker.cshtml should resolve the cultures like before, and then invoke a new ContentCulturePickerContainer with this model. The driver can then use the ContentCulturePickerContainer instead, and rendering ContentCulturePicker will work like before.

no, it does not in 2.0.0-preview-18222:
before, it was possible to override the shape with liquid and get all the required data as part of the Model.

I guess the problem is, that ContentCulturePickerContainer is not invoked anymore, so you directly need to override ContentCulturePicker

As the ContentCulturePicker does not work with a Model, but creates variables with the required data inside the template itself, there is no way to access this data inside a liquid template, which overrides the template.

Am I missing a way to call this in liquid to get the equivalent for Model.SupportedCultures?
var supportedCultures = (await LocalizationService.GetSupportedCulturesAsync()).Select(c => CultureInfo.GetCultureInfo(c));

Originally posted by @bashuss in #15148 (comment)

@Piedone
Copy link
Member

Piedone commented May 31, 2024

Please elaborate and use the template, because at this point, this issue is not yet actionable.

@bashuss
Copy link
Author

bashuss commented May 31, 2024

Currently we only have this template to override:

The ContentCulturePickerContainer, which was available earlier, had a Model which allowed liquid templates like this:

[...]
<li class="nav-item dropdown">
    <a class="nav-link dropdown-toggle" href="#" id="oc-culture-picker" data-bs-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
        <i class="fas fa-globe" title="{{Model.CurrentCulture.NativeName | t}}"></i>
        <span class="culture-drop-text">{{ cur_lang }}</span>
        </a>
    <ul class="dropdown-menu" aria-labelledby="oc-culture-picker">
    {% for culture in Model.SupportedCultures %}
        {% assign nname = culture.NativeName | split: " " %}
        {% if culture.Name != Model.CurrentCulture.Name  %}
        <li class="nav-link"><a class="dropdown-item" href="{{culture.Name | switch_culture_url }}">{{nname[0]}}</a></li>
        {% else %}
        <li class="nav-link"><span class="culture-select-text"><i class="fas fa-check"></i> {{ nname[0] }}</span></li>
        {% endif %}
    {% endfor %}
    </ul>
</li>

ContentCulturePicker template does not provide a ViewModel (Model.SupportedCultures, Model.CurrentCulture) that would allow a template like the given one. I may be missing some liquid calls, which would allow that, but I cannot see them in the documentation.

@bashuss
Copy link
Author

bashuss commented May 31, 2024

ContentCulturePicker.cshtml should be something like this:

@using Microsoft.AspNetCore.Localization
@using OrchardCore.Localization
@using System.Globalization

@inject ILocalizationService LocalizationService
@{
    var supportedCultures = (await LocalizationService.GetSupportedCulturesAsync()).Select(c => CultureInfo.GetCultureInfo(c));

    if (supportedCultures.Count() < 2)
    {
        return;
    }

    var currentCulture = Context.Request.HttpContext.Features
            .Get<IRequestCultureFeature>()?.RequestCulture?.Culture ?? CultureInfo.CurrentUICulture;
}

followed by a call to the ContentCulturePickerContainer template, with supportedCultures and currentCultures as Properties of the Model.

ContentCulturePickerContainer should then contain the rest of the current template - something like this:

<li class="nav-item dropdown">
    <a class="nav-link dropdown-toggle" href="#" id="oc-culture-picker" data-bs-toggle="dropdown" aria-haspopup="true" aria-expanded="false">@Model.CurrentCulture.NativeName</a>
    <div class="dropdown-menu" aria-labelledby="oc-culture-picker">
        @foreach (var culture in Model.SupportedCultures)
        {
            if (culture.Name == Model.CurrentCulture.Name)
            {
                continue;
            }

            <a asp-route="RedirectToLocalizedContent"
               asp-route-area="OrchardCore.ContentLocalization"
               asp-route-targetCulture="@culture.Name"
               asp-route-contentItemUrl="@Context.Request.Path.Value"
               asp-route-queryStringValue="@Context.Request.QueryString.Value"
               class="dropdown-item">@culture.NativeName</a>
        }
    </div>
</li>

This would reenable overriding with liquid. Why was that dropped in the first place?

@Piedone Piedone changed the title no overriding of contentCulturePicker template possible in liquid Hard to implement a ContentCulturePicker in Liquid May 31, 2024
@Piedone
Copy link
Member

Piedone commented May 31, 2024

OK, so the point as far as I understand is that while you can override ContentCulturePicker from Liquid, you can't build the exact equivalent of the Razor implementation, because there's no built-in way to recreate this line:

 var supportedCultures = (await LocalizationService.GetSupportedCulturesAsync()).Select(c => CultureInfo.GetCultureInfo(c));

(Note that fetching the current culture is possible, see https://docs.orchardcore.net/en/latest/reference/modules/Liquid/#culture.)

Without the changes in ContentCulturePickerNavbarDisplayDriver, this wouldn't be necessary, since before that, it was the driver that produced both supportedCultures and currentCulture . @MikeAlhayek can you shed some light on why this specific change was necessary?

@MikeAlhayek
Copy link
Member

What I think we need is a way using Liquid to return culture info. I don’t have time to look now, @Piedone do you know if we have a Culture info liquid object?

We either need a function or a filter that would return the following info:

Model.SupportedCultures?
var supportedCultures = (await LocalizationService.GetSupportedCulturesAsync()).Select(c => CultureInfo.GetCultureInfo(c));

@MikeAlhayek
Copy link
Member

Th PR #16208 adds a filter that would provide you the supported cultures.

@Piedone
Copy link
Member

Piedone commented May 31, 2024

While that's great, why can't the driver produce these values, just as before?

@MikeAlhayek
Copy link
Member

MikeAlhayek commented May 31, 2024

If I recall correctly, in the past ContentCulturePicker received a shape not a view model. It may break others if we change the behavior. I don't use Liquid much, if we pass model to the view, will we be able to access that from Liquid?

@Piedone
Copy link
Member

Piedone commented May 31, 2024

If we do it with the dynamically added properties like they were then yes. So, basically revert this and the corresponding template change and I think we're good.

Having a Liquid filter in addition to this is nice to have, but this would make it easier to simply override this shape (also from Razor BTW). Unless there's a good reason not to let the driver produce the two values.

@MikeAlhayek
Copy link
Member

I am not sure what part to roll back so this issue is solved. If you have a suggestion, maybe a PR will help.

@Piedone
Copy link
Member

Piedone commented Jun 1, 2024

The linked driver and corresponding template changes. The again, was there a particular reason for these?

@MikeAlhayek
Copy link
Member

Yes the change is because we now have Navbar shape that is rendered in the top. The the driver what injects the template as a navbaritem. I don’t think you should change the driver or the template. Most people are not impacted by that change. The only impact of one is overriding that template using Liquid, and now they can with the new PR. Before we have a container view that was injected using a filter which creates a picker shape

@Piedone
Copy link
Member

Piedone commented Jun 1, 2024

But why does that affect if this driver can pass parameters to the shape?

@MikeAlhayek
Copy link
Member

I recall the details. It should not. But the fact that was changed indicates there was a reason for it. Feel free to try it out and see if anything breaks. I would setup a site using 1.8 and then run the 2.0 code on it.

@bashuss
Copy link
Author

bashuss commented Jun 2, 2024

{% assign cultures = Culture | supported_cultures %}
should this already work in 2.0.0-preview-18229 - if it should, it does not work for me.

(Note that fetching the current culture is possible, see https://docs.orchardcore.net/en/latest/reference/modules/Liquid/#culture.)

Not the same thing as Model.CurrentCulture, as this had at least the properties Model.CurrentCulture.Name and Model.CurrentCulture.NativeName and maybe more.
{{ Culture.NativeName }} currently results in null

So you cannot access the NativeName using liquid currently.

@MikeAlhayek
Copy link
Member

@bashuss the PR was not yet merged into main so it will not work until after the PR is merged. I have added more properties like DisplayName and NativeName of the current culture.

@bashuss
Copy link
Author

bashuss commented Jun 3, 2024

@MikeAlhayek Thanks for the info - I still do not really know my way around gitHub. If I would, I may even directly contribute a bit. Thanks for adding the properties!

@Piedone
Copy link
Member

Piedone commented Jun 3, 2024

This works with that PR, BTW:

<li class="nav-item dropdown">
    <a class="nav-link dropdown-toggle" href="#" id="oc-culture-picker" data-bs-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
        {{ Culture.NativeName }}
    </a>
    <div class="dropdown-menu" aria-labelledby="oc-culture-picker">
        {% assign cultures = Culture | supported_cultures %}
        {% for culture in cultures %}
            {% if culture.Name == Culture.Name %}
                {% continue %}
            {% endif %}

            <a class="dropdown-item">{{ culture.NativeName }}</a>
        {% endfor %}
    </div>
</li>

I didn't spend time on the link, that's pretty standard.

But I still argue we should revert the driver change: #16208 (comment)

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

Successfully merging a pull request may close this issue.

3 participants