jazzband / wagtailmenus

An app to help you manage and render menus in your Wagtail projects more effectively
MIT License
396 stars 138 forks source link

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

Open ababic opened 6 years ago

ababic commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

robnardo commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 :smiley:

dwasyl commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

perepicornell commented 3 years ago

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 commented 3 years ago

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

ababic commented 3 years ago

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 commented 3 years ago

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

perepicornell commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 :smile:

ababic commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

Redjam commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

Redjam commented 2 years ago

@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 commented 1 year ago

@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 commented 1 year ago

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.

goutnet commented 7 months ago

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 commented 7 months ago

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