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

Feature suggestion: Make wagtailmenus more useful in multilingual projects where multiple languages share the same Site #242

Open
ababic opened this issue May 8, 2018 · 29 comments

Comments

@ababic
Copy link
Collaborator

ababic commented May 8, 2018

I'm considering adding a language field to both AbstractMainMenu and AbstractFlatMenu, which would default to the default language for the project, then altering the unique constraints on each model to allow menus to be created for unique combinations of site and language (or site, language and handle in the case of AbstractFlatMenu).

The {% main_menu %} and {% flat_menu %} tags and get_for_site() methods on the related models should then be updated to identify the current language and use it to identify the relevant menu object.

This behaviour would perhaps be 'toggleable' via a new setting (disabled by default) so that users wouldn't have to be concerned.

This same setting could also be used to conditionally customise the CMS UI, revealing the language field for flat menus in the add/edit UI, and also adding a language filter to the filters list.

The UI for main menus presents a more complicated problem. Right now, using the 'site switcher' select in the corner always creates a menu for the selected site if it didn't already exist, which I'm not sure is the best way to cater for multiple languages - I think the UI might need need to indicate more to the user than a single select can provide concisely, like which sites menus for which languages already, and which don't. Again, this will have to be explored.

@ababic ababic changed the title Investigate how wagtailmenus can be made more useful for multilingual projects, where the same site is used for each language Investigate how wagtailmenus can be made more useful for multilingual projects, where multiple languages share the same Site May 8, 2018
@ababic ababic changed the title Investigate how wagtailmenus can be made more useful for multilingual projects, where multiple languages share the same Site Investigate how wagtailmenus can be made more useful for multilingual projects where multiple languages share the same Site May 8, 2018
@ababic
Copy link
Collaborator Author

ababic commented May 14, 2018

I think the aim is 'out-of-the-box' compatibility with something like wagtailtrans, but if the two apps were used in conjunction, the developer would want the 'language' field to be a ForeignKey to the Language model, which isn't something I'm keen to change on the default models.

Perhaps the best approach here will be to create an optional sub-app within wagtailmenus (e.g. wagtailmenus.contrib.wagtailtrans), and developers would deliberately install that when working with wagtailtrans to activate the updated models.

@ababic
Copy link
Collaborator Author

ababic commented Jul 27, 2018

Now that #258 is resolved, I'll try to make some progress with this soon :)

@ababic ababic changed the title Investigate how wagtailmenus can be made more useful for multilingual projects where multiple languages share the same Site Feature suggestion: Make wagtailmenus more useful in multilingual projects where multiple languages share the same Site Aug 1, 2018
@ababic ababic added this to the Some time in the future milestone Oct 8, 2018
@ababic ababic closed this as completed Jun 1, 2019
@robnardo
Copy link

robnardo commented Jan 3, 2020

hey @ababic - i would be interested in this. One possible solution could be to have multiple main menus. And add a handle field for each main menu created. Then it would be up to the template to decide which one to use, like so:
{% main_menu handle="my_en_menu" %} or {% main_menu handle="my_fr_menu" %}

Flat menu seems to have similar functionality - can you maybe just add that to Main menu?

@robnardo
Copy link

robnardo commented Jan 3, 2020

actually.. if the Flat Menu has all the fields, then i could just do it using Flat menu.. hmm. I will give it a try in a few days.

@ababic
Copy link
Collaborator Author

ababic commented Jan 4, 2020

@robnardo I think using flat menus is a common way around this problem, although it's not great for developers to have to assemble the correct handles every time. It would be much nicer to use the active language to get the correct version automatically.

I've closed this just because I haven't had the free time to spend on feature development for a while, but I would be happy to open this issue again if you think it would be useful. I suppose someone may pick it up an submit a PR one day 😃

@ababic ababic reopened this Jan 4, 2020
@dwasyl
Copy link

dwasyl commented Jan 10, 2020

Someting like this would be useful. In my solutions I've used wagtail-modeltranslation which is based on django-modeltranslation and just registered the MainMenuItem and FlatMenuItem for translation as third party models. It's not ideal, but it's worked well. Something more integrated would be great, so long as it all fit together, no matter which translation app you use with Wagtail.

@ababic
Copy link
Collaborator Author

ababic commented Jun 27, 2020

I think wagtail is close to figuring out what it wants to do natively for translations/localisation, so it's probably worth holding out for that.

@calbear47
Copy link

How is it suggested we deal with menus when we have regional websites at different subpaths.

We have our North American English at the path /enbut we also want to have another english website for Europe under the path /eu.

How can we construct multiple "main menus" for each regional website. Should we not use main menus at all and construct flat menus for each regional main menu? Any thoughts would be greatly appreciated.

@ababic
Copy link
Collaborator Author

ababic commented Oct 26, 2020

Hey @calbear47. That isn't really something wagtailmenus caters for out-of-the-box. I think main menus could work, but you'd need a custom menu model (with relationships/unique criteria overridden, and certain methods overridden to take into account the 'current region' from the request), plus custom admin views for switching between different site/region combinations in the CMS.

@ababic
Copy link
Collaborator Author

ababic commented Oct 26, 2020

@calbear47 Whatever native solution we come up with for wagtailmenus will almost certainly now be inspired by the native localisation models/features coming in Wagtail 2.11

@calbear47
Copy link

@ababic Ok, thank you for your input and direction.

@perepicornell
Copy link

I'm using the official wagtail approach to i18n, and for the menus to work I followed these steps:
https://wagtailmenus.readthedocs.io/en/stable/advanced_topics/custom_menu_classes.html#replacing-both-the-mainmenu-and-mainmenuitem-models

And for the new classes I put:

# I don't like that in the example they import this as "settings"
from wagtailmenus.conf import settings as wagtail_settings


class LocalizedMainMenu(AbstractMainMenu):
    pass


class LocalizedMainMenuItem(AbstractMainMenuItem):
    menu = ParentalKey(
        LocalizedMainMenu,
        on_delete=models.CASCADE,
        related_name=wagtail_settings.MAIN_MENU_ITEMS_RELATED_NAME,
    )

    def __init__(self, *args, **kwargs):
        """
        This makes the menu work seamlessly with multi-language entirely:
        - In the admin you only add the pages once per language, but the
          menu also appears even if you are visiting the page that is in
          another language.
        - The menu item's text is in the current language.
        - The menu item's link point to the right language.
        """
        super().__init__(*args, **kwargs)
        if self.link_page and self.link_page.localized:
            self.link_page = self.link_page.localized

Then, just create the main menu as you would normally, that is: each menu item just once for each page, and select the page in one of the languages (any should work, but I always select the main one).

Until wagtailmenus officially supports wagtail's i18n, this might be a temporary solution.

@ababic
Copy link
Collaborator Author

ababic commented Jan 28, 2021

Thanks for sharing @perepicornell! That's superb :)

@ababic
Copy link
Collaborator Author

ababic commented Jan 28, 2021

My only concern there would what happens when you edit and resave the menu. Don't the stored pk values change depending on what language is active? Does that have any negative impact?

@ababic
Copy link
Collaborator Author

ababic commented Jan 29, 2021

Overriding get_pages_for_display() might be a safer bet, but would take a bit more custom code.

@perepicornell
Copy link

That's a very good point, I didn't verify it but saving a menu in admin will override the pages to the one of the language you are using.
I agree that this way is reckless, but so far the menu is going to work the same way regardless of the language that you choose for the page, also I imagine that an update for wagtailmenus to officially support the wagtail's new i18n will come in the near future, so for a temporary workaround I'm happy enough :D

And yes, I was taking a look at get_pages_for_display but I got overwhelmed and took the shortcut..

@perepicornell
Copy link

Hi! I'm sorry I don't fully understand the issue, but it's working like a charm for me. If it helps, I'm only using the locale codes so far, so the language switching menu says "CA - ES - EN".

@perepicornell
Copy link

perepicornell commented Mar 10, 2021

Hi again!

The trick of extending AbstactMainMenuItem to change its self.link_page to self.link_page.localized introduces a bug that prevents menu items with subpages to be displayed.

Say you save a menu while using the admin in english language, now in the Menu model you have the english versions of the Page objects.
Let's say the page is "Projects" with ID 30 in english.

Then you visit the site in catalan, now the page that was ID 30 in english is ID 8 in catalan.

MenuWithMenuItems.get_top_level_items() will be called and will do this:
menu_items = self.get_base_menuitem_queryset()
Now you have all the menu items in the Menu model in their localized version, that is, in catalan, so "Projects" in menu_items has id 8.
Then it will compare menu_items items with get_pages_for_display() items. The latest will contain the pages already localized, except if the menu item allows submenus, in that case IT WILL NOT. Instead will use the stored one in english, which ID is 30 and not 8.

That's when it checks this:

try:
    item.link_page = self.pages_for_display[item.link_page_id]

Which, in the case described, fails and the menu item is not displayed.

I fixed in by overriding get_pages_for_display() and adding:

            if item.link_page.localized:
                item.link_page = item.link_page.localized

And the final result is this:

class LocalizedMainMenu(AbstractMainMenu):

    def get_pages_for_display(self):
        """Returns a queryset of all pages needed to render the menu."""
        if hasattr(self, '_raw_menu_items'):
            # get_top_level_items() may have set this
            menu_items = self._raw_menu_items
        else:
            menu_items = self.get_base_menuitem_queryset()

        # Start with an empty queryset, and expand as needed
        queryset = Page.objects.none()

        for item in (item for item in menu_items if item.link_page):
            if item.link_page.localized:
                item.link_page = item.link_page.localized

            if(
                item.allow_subnav and
                item.link_page.depth >= wagtail_settings.SECTION_ROOT_DEPTH
            ):
                # Add this branch to the overall `queryset`
                queryset = queryset | Page.objects.filter(
                    path__startswith=item.link_page.path,
                    depth__lt=item.link_page.depth + self.max_levels,
                )
            else:
                # Add this page only to the overall `queryset`
                queryset = queryset | Page.objects.filter(id=item.link_page_id)

        # Filter out pages unsutable display
        queryset = self.get_base_page_queryset() & queryset

        # Always return 'specific' page instances
        return queryset.specific()


class LocalizedMainMenuItem(AbstractMainMenuItem):
    menu = ParentalKey(
        LocalizedMainMenu,
        on_delete=models.CASCADE,
        related_name=wagtail_settings.MAIN_MENU_ITEMS_RELATED_NAME,
    )

    def __init__(self, *args, **kwargs):
        """
        This makes the menu work seamlessly with multi-language entirely:
        - In the admin you only add the pages once per language, but the
          menu also appears even if you are visiting the page that is in
          another language.
        - The menu item's text is in the current language.
        - The menu item's link point to the right language.
        """
        super().__init__(*args, **kwargs)
        if self.link_page and self.link_page.localized:
            self.link_page = self.link_page.localized

I know it's not the right way (I would normally never override a method by copying its entire code), but it was already very hard for me to understand all of that and find a workaround because I'm not that good at it yet.

Now it works fine, but if anyone can help me in finding the right way for solving this I will really appreciate it!

Many thanks 😄

@ababic
Copy link
Collaborator Author

ababic commented Mar 10, 2021

Hey @perepicornell. I'm sure this will be really helpful for others... thanks for doing such a great job of explaining the problem and your solution.

@noparamos
Copy link

noparamos commented Mar 10, 2021

Good day @perepicornell! Thats what i meant:). Thank you, works great, especially after commenting string super().get_top_level_items(), because it causes recursional reqests therefore my DB goes down:

Mar 10 15:04:08 ant gunicorn[245124]: File "/var/www/example.com/portfolio/home/models.py", line 297 in get_pages_for_display
Mar 10 15:04:08 ant gunicorn[245124]: File "/var/www/example.com/env/lib/python3.6/site-packages/wagtailmenus/models/menus.py", line 284 in pages_for_display
Mar 10 15:04:08 ant gunicorn[245124]: File "/var/www/example.com/env/lib/python3.6/site-packages/django/utils/functional.py", line 48 in get
Mar 10 15:04:08 ant gunicorn[245124]: File "/var/www/example.com/env/lib/python3.6/site-packages/wagtailmenus/models/menus.py", line 971 in get_top_level_items
Mar 10 15:04:08 ant gunicorn[245124]: File "/var/www/example.com/portfolio/home/models.py", line 297 in get_pages_for_display

@perepicornell
Copy link

Thanks @noparamos ! Damn, I put this line just to jump to the reference easily in PyCharm and I forgot to remove it before copying 😅
fixed!

@Redjam
Copy link

Redjam commented Feb 24, 2022

Hi there, I use Wagtail-Localize and Wagtailmenus together. I found a solution to make the menu localized but it's so easy that I'm pretty sure I have forgotten something 😅.

I have created a Main Menu and the following template:

{% load i18n menu_tags %}

<ul class="font-title text-black text-2xl tracking-wider space-y-8">
    {% for item in menu_items %}
        <li class="relative px-4">
            <a class="before:w-12 before:h-0.5 before:bg-black before:absolute before:-bottom-2.5 before:left-4 before:opacity-0 before:origin-left before:scale-x-0 before:transition-transform before:duration-500 hover:before:scale-x-100 hover:before:opacity-100"
               href="{{ item.link_page.localized.url }}">{{ item.link_page.localized.title }}</a>
        </li>
    {% endfor %}
</ul>

Using link_page.localized, the url and title that are displayed depend on the location code.

I only create one menu and let the translation happen in the template.

I don't beleive this workarround to be so easy. Where did I miss something? 😂

@ababic
Copy link
Collaborator Author

ababic commented Feb 24, 2022

@Redjam I would have a scan at the code suggestions further up, specifically this one. Wagtailmenus does some pre-fetching of page data to use for rendering, so ideally you need to swap the pages out before it gets to the template, so that the correct pages are prefetched in the first place. Your solution will work for a simple menu, but not for one with multiple levels, because the localized page descendants will not have been prefetched.

@Redjam
Copy link

Redjam commented Feb 24, 2022

@ababic many thanks for your feedback. I'll dig into the subject to improve my code.

@Redjam
Copy link

Redjam commented Feb 25, 2022

@perepicornell trick didn't work for me.

I struggled to debug but I found what was causing the issue.

In the line queryset = self.get_base_page_queryset() & queryset I had to replace & by and to make it work.

I can't explain why I had to make this change 🤷🏻‍♂️ Maybe it will help someone else.

Pyhton version used: Python 3.8.12

@fpennica
Copy link

fpennica commented Mar 9, 2023

@perepicornell trick didn't work for me.

I struggled to debug but I found what was causing the issue.

In the line queryset = self.get_base_page_queryset() & queryset I had to replace & by and to make it work.

I can't explain why I had to make this change 🤷🏻‍♂️ Maybe it will help someone else.

Pyhton version used: Python 3.8.12

I'm using wagtail-localize too, the only problem with this is that switching to a different language than that of the pages chosen for the main menu, the menu is not rendered.

The problem is in queryset = self.get_base_page_queryset() & queryset, but replacing the operator & with and cannot work.

If I understand correctly, get_base_page_queryset() returns only the pages for the language chosen when building the menu, while queryset contains pages filtered for the current language. If the languages are different the result of self.get_base_page_queryset() & queryset is empty.

Until there is better solution, a quick & dirty workaround could be replacing the line with something like:

base_page_queryset = self.get_base_page_queryset()
suitable_pages_ids = [p.localized.id for p in base_page_queryset]
queryset = queryset.filter(id__in=suitable_pages_ids)

@hoiwanchang
Copy link

With replace & with and like Redjam

Here is a working solution for me on main menu, I don't know it is OK or not.

  • in a level 2 template.
{% load menu_tags %}
{% for item in menu_items %}
        <a class="navbar-item {{ item.active_class }}" href="{{ item.href }}">{{ item.text }}</a>
{% endfor %}
  • in menus.py Overwriting _prime_menu_item()
# wagtailsmenus/models/menus.py

        # ---------------------------------------------------------------------
        # Set 'text' attribute
        # ---------------------------------------------------------------------

        if item_is_menu_item_object:
            item.text = item.menu_text
        else:
            # i18n update, since page always return title as the default value.
            if page.localized:
                item.text = str(page)
            else:
                item.text = getattr(item, settings.PAGE_FIELD_FOR_MENU_ITEM_TEXT, item.title)

        # ---------------------------------------------------------------------
        # Set 'href' attribute
        # ---------------------------------------------------------------------

        if option_vals.use_absolute_page_urls:
            item.href = item.get_full_url(request=request)
        else:
            # i18n update, also we can get the url of the localized page.
            if page.localized:
                item.href = page.localized.url
            else:
                item.href = item.relative_url(current_site, request=request)

@goutnet
Copy link

goutnet commented Feb 13, 2024

Hi there. Should there be a pull request with this?

This would be rather useful in the mainline. @hoiwanchang ?

@ababic, since this issue has been open for about six years, are you aware of an effort to make it work in the mainline?

Thanks,

@ababic
Copy link
Collaborator Author

ababic commented Feb 13, 2024

Hi @goutnet. Not that I know of. I could never really settle on an approach for multilingual projects, and eventually abandoned the idea..

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

No branches or pull requests

10 participants