ietf-tools / www

A customized CMS for the IETF website
BSD 3-Clause "New" or "Revised" License
22 stars 45 forks source link

feat: Megamenu #367

Closed mgax closed 7 months ago

mgax commented 7 months ago

Fixes https://github.com/ietf-tools/wagtail_website/issues/326

Menu logic

Screenshots

kesara commented 7 months ago

@mgax In your PR description, did you forget to add the screenshots?

mgax commented 7 months ago

@mgax In your PR description, did you forget to add the screenshots?

Yes indeed, fixed, thanks!

kesara commented 7 months ago

This change doesn't work with IAB website. Because when IAB layout is selected sub menus don't work. See: (with IAB layout for IAB site)

Screenshot 2023-12-22 at 00 33 24

(with IETF layout for IAB site)

Screenshot 2023-12-22 at 00 40 26

The new IAB site will have submenus as well: https://temporary.iab.org/

mgax commented 7 months ago

This change doesn't work with IAB website. Because when IAB layout is selected sub menus don't work.

That's weird, I've tested locally with a fresh restore of the IAB database, and the current megamenu branch (commit 0b257a7), and the menu looks just like on https://temporary.iab.org.

On this PR, IAB used to be broken, but commit 398561d fixed it. Looking at the 2nd screenshot, (with IETF layout for IAB site):

292192826-03395561-c418-4a0b-a5d0-905074e3f395

It seems there are MainMenuItem snippets in the database, which shouldn't be the case – commit 398561d changed the migration to only create MainMenuItem records if there is an home.HomePage page present, and the IAB database (at least, my copy of it) doesn't have one.

@kesara I suspect an earlier version of this branch was deployed to the IAB instance, so an older version of the migration utils.0009_megamenu was run. Could you please make sure the repository is at commit 0b257a7, then roll back the migration, and apply it again?

./manage.py migrate utils 0007
./manage.py migrate
kesara commented 7 months ago

It seems there are MainMenuItem snippets in the database, which shouldn't be the case – commit 398561d changed the migration to only create MainMenuItem records if there is an home.HomePage page present, and the IAB database (at least, my copy of it) doesn't have one.

@mgax I had the up-to-date branch, but I ran it as a multisite where I had an IETF database and created a separate IAB Home for testing. I'll go forward with the current changes. But it would be nice to have a mega menu that will work when there a multiple sites (both IETF & IAB varients) on the same instance.

mgax commented 7 months ago

But it would be nice to have a mega menu that will work when there a multiple sites (both IETF & IAB varients) on the same instance.

It should work though. Perhaps the if that checks whether the current website has the IETF or IAB theme could be improved. The one for the megamenu is the same as the one which was already in place for the home page and the secondary menu. My suggestion would be to use ietf.utils.models. LayoutSettings to make the decision.

kesara commented 7 months ago

Yeah, LayoutSettings would be best thing to use.