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

Culture Picker is not rendering cultures after upgrading to v1.8.2 #15148

Closed
tonydev24 opened this issue Jan 22, 2024 · 29 comments · Fixed by #15532
Closed

Culture Picker is not rendering cultures after upgrading to v1.8.2 #15148

tonydev24 opened this issue Jan 22, 2024 · 29 comments · Fixed by #15532
Labels
Milestone

Comments

@tonydev24
Copy link

Describe the bug

After upgrading the project from 1.7.2 to 1.8.2 I had to change the culture picker template name and it's not showing the current culture and supported cultures data.

To Reproduce

Steps to reproduce the behavior:

  1. Create a new project in version 1.7.2 using the orchard template and create a site using the agency theme.
    image
    image

  2. Add a template named ContentCulturePickerContainer to show the current culture and supported cultures.
    image

  3. Add a new template named Layout to override the theme layout and add in it a shape to show the culture picker template using this line: {% shape "ContentCulturePicker" %}
    image

  4. The culture picker shows data on the page.
    image

  5. Upgrade the project to 1.8.2.
    image

  6. An error is shown in the page.
    image

  7. Change the culture picker template name to ContentCulturePicker and the error disappears and the page opens normally but the culture picker does not render data on the page.
    image
    image

Expected behavior

Show current culture and the supported cultures in the top menu.

@hishamco
Copy link
Member

hishamco commented Jan 22, 2024

I will try to reproduce the issue, and then fix it unless you want to submit a PR

@tonydev24
Copy link
Author

Thanks I will be waiting for the fix. I won't be submitting a PR. Regards

@hishamco
Copy link
Member

I won't be submitting a PR. Regards

Why not :) hope to get time today coz I have many pending PRs to update

@hishamco
Copy link
Member

@tonynasr please share a minimal repo to save my time, so I can give this some priority

@tonydev24
Copy link
Author

OrchardCore.Cms.Web2.zip

@jagbarcelo
Copy link
Contributor

I can confirm that the problem was already there in v1.8.1. We updated to 1.8.1 (not 1.8.2 yet) and the same exception is shown.

@tonydev24
Copy link
Author

Yes I also confirm that it was already there in 1.8.0

@hishamco
Copy link
Member

@MikeAlhayek we might need another patch 1.8.3 :)

@hishamco
Copy link
Member

I can reproduce the issue

@hishamco
Copy link
Member

@MikeAlhayek seems ContentCulturePickerNavbarDisplayDriver.DisplayAsync() has never been called I remember there's a refactor that you made to almost if not all the shape drivers, I'm not sure if that's the issue or something related

@hishamco
Copy link
Member

The NavBar introduced in the 1.8.0 #14488, @MikeAlhayek it's in your plate now

@MikeAlhayek
Copy link
Member

@tonydev24 @hishamco
You don't need to add that template. Please review the upgrade notes for 1.8. You need to build and inject the Navbar shape as we do here

var navbar = await DisplayAsync(await DisplayManager.BuildDisplayAsync<Navbar>(UpdateModelAccessor.ModelUpdater));

Then

You don't need to add ContentCukturePicker unless you want to override its style.

@tonydev24
Copy link
Author

Yes I noticed the new navbar but we need to override its style so we need to add the mentioned template.
Also is there a way to add the navbar to a liquid template if we needed to use the existing navbar as is?
And when rendering it in the custom template, is there a way in liquid to render the Arabic culture name in arabic "العربية" as it is rendered in the new navbar which appears in the default theme?
Thanks

@MikeAlhayek
Copy link
Member

In liquid you have helpers to create shapes. So you would probably need to create the Navbar shape from the layout in razor. This shape should be created very early in the layout and then rendered where you want it to show up.

You should be able to print a literal string without adding "| s"

{{ "العربيه" }}

@MikeAlhayek
Copy link
Member

@tonydev24 is this still any issue or can we close it?

@tonydev24
Copy link
Author

Yes the issue is still there, Culture Picker is not rendering cultures when used in a liquid template as mentioned in the ticket. Thanks

@sebastienros sebastienros modified the milestones: 1.x, 1.9 Feb 8, 2024
@bashuss
Copy link

bashuss commented Mar 4, 2024

I have the same problem after upgrade from 1.7.2 to 1.8.2.

I use a layout.liquid file currently. It would be nice, if the usage of the new navbar would be documented for liquid and razor.
Currently I inject a menu shape with a custom settings configured menu in layout.liquid. This can be overridden from a ContentItem template. This works fine, but obviously needs this bug to be fixed.

I like to try navbar and see if there would be a solution, but I could not find a proper documentation for liquid.

@hishamco
Copy link
Member

hishamco commented Mar 5, 2024

@MikeAlhayek could you please handle this, seems I'm another one lost with CulturePicker after introducing the NavBar :)

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Mar 5, 2024

I have not tried to create this using Liquid. If someone does this, please share the code.

But, in version 1.8, the we will no longer inject the drop down menu into the NavbarTop zone. In face, that zone is removed from OC themes. So if you are using a theme that needs the culture picker, you'll have to to inject the Navbar shape where you want it to show up.

I am guessing something like {% assign navbarShape = "Navbar" | shape_new %} is what you need to do to create the shape using liquid.

Then where you want to render it in the layout, you'll need to add {{ navbarShape | shape_render }}. This should be enough to render the Navbar in your theme.

Having the Navbar shape will allow us to inject the ContentCulturePicker.cshtml view into the theme when the OrchardCore.ContentLocalization.ContentCulturePicker feature is enabled.

@hishamco
Copy link
Member

hishamco commented Mar 5, 2024

How we can do it in Razor?

I have not tried to create this using Liquid. If someone does this, please share the code.

I will do once I know the required changes

@MikeAlhayek
Copy link
Member

Look at the changes in TheTheme layout.

First we create the Navbar shape

https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Themes/TheTheme/Views/Layout.cshtml#L14

We later render it here

https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Themes/TheTheme/Views/Layout.cshtml#L54

Now if you enable the OrchardCore.ContentLocalization.ContentCulturePicker you should see the picker.

@hishamco
Copy link
Member

hishamco commented Mar 5, 2024

I remember in Liquid we needed to add a ContentCulturePicker shape before, so I think I need to add the Navbar shape first, right?

@MikeAlhayek
Copy link
Member

You don't need ContentCulturePicker as it should get injected automatically when you enable the feature.

All you should need is creating the Navbar shape and rendering it.

@bashuss
Copy link

bashuss commented Mar 10, 2024

@MikeAlhayek : I still don't really get, what NavBar actually does. It does not render our menu, does it?

Rendering the menu is done by this line, correct?
https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Themes/TheTheme/Views/Layout.cshtml#L53

And NavBar is injected after rendering the menu.

I like to have the ContentCulturePicker rendered like every other MenuItem as Part of my menu. I do not want a different treatment in my Html for ContentCulturePicker and the MenuItems coming from my menu.

Will NavBar do that for me? Currently I don't see how, unless it collects all MenuItems rendered before NavBar is inserted.

Basically I don't see a way, how NavBar will help me to fix my ContentCulturePicker without breaking my css styling.

@MikeAlhayek
Copy link
Member

By default the Navbar is an additional menu that is rendered next to the main-menu. It is rendered by @Navbar which is generated earlier on line 14

https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Themes/TheTheme/Views/Layout.cshtml#L14

If you want to merge the items generated by main-menu and Navbar, you could wrap the two with a

    tag in the layout file. Then change the Admin-AdminMenu.cshtml and the Shape.cshtml to not render a separate
      tag.

      The Navbar will also render the logged in user menu, and other items by default such as notification menu. However, you can use placement to not show some items if you want to not show them.

@bashuss
Copy link

bashuss commented Mar 15, 2024

I have not tried to create this using Liquid. If someone does this, please share the code.

But, in version 1.8, the we will no longer inject the drop down menu into the NavbarTop zone. In face, that zone is removed from OC themes. So if you are using a theme that needs the culture picker, you'll have to to inject the Navbar shape where you want it to show up.

I am guessing something like {% assign navbarShape = "Navbar" | shape_new %} is what you need to do to create the shape using liquid.

Then where you want to render it in the layout, you'll need to add {{ navbarShape | shape_render }}. This should be enough to render the Navbar in your theme.

Having the Navbar shape will allow us to inject the ContentCulturePicker.cshtml view into the theme when the OrchardCore.ContentLocalization.ContentCulturePicker feature is enabled.

These code snippets do not work inside my layout.liquid. Nothing happens. Also the generated shape seems to be empty.
Is there anything I need to configure apart from enabling the ContentCulturePicker? Or do I have to disable and reenable it?

@sebastienros
Copy link
Member

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.

@MikeAlhayek
Copy link
Member

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.

With PR #15532 users can create ContentCulturePicker shape and the cultures picker should show up. For example {% shape "ContentCulturePicker" %}

However, a new Liquid function for navbar was added to make it easier to inject the Navbar into your own theme. To do that,

  1. Construct the shape at the beginning of the layout.liquid file to enable navbar items to potentially contribute to the resources output as necessary.
{% assign navbar = Navbar() | shape_render %}
  1. Subsequently in the layout.liquid file, invoke the shape at the location where you want to display it.
{{ navbar }}

@bashuss
Copy link

bashuss commented May 28, 2024

@sebastienros

ContentCulturePicker.cshtml should resolve the cultures like before, ...

not, 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));

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

Successfully merging a pull request may close this issue.

6 participants