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

Main Menu #41

Merged
merged 75 commits into from
Jan 29, 2025
Merged

Main Menu #41

merged 75 commits into from
Jan 29, 2025

Conversation

sanjeevz3009
Copy link
Contributor

@sanjeevz3009 sanjeevz3009 commented Nov 26, 2024

What is the context of this PR?

Add a navigation app in Wagtail to manage the main menu, allowing for structured, customisable navigation.

How to review

  • Spin up the ONS BETA Web App Locally as normal.
  • Snippets -> Main Menu -> Add main menu.
  • Fill in the menu content -> After saving every time you should see the preview on the preview panel.
  • Go to Settings -> Navigation Settings -> Choose main menu -> Choose the menu shown so it can be displayed on the website.

Loom walkthrough of the main menu: Main Menu Features Loom Walkthrough

Credit: @zerolab for the help and guidance.

Follow-up Actions

Fix column alignment issues (can be seen during the loom video): #63

@sanjeevz3009 sanjeevz3009 changed the title Main Menu Main Menu - Back-end Nov 26, 2024
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a draft. Thought I'd leave a few notes to boot

…ed, section restricted to 3, URL label added, icon for navigation settings added and restricting it to so only 1 main menu can be created
Copy link
Contributor

@helenb helenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sanjeevz3009 I have tested locally and it all functions well. A couple of small comments on the validation and example help text.

@sanjeevz3009
Copy link
Contributor Author

Thanks @sanjeevz3009 I have tested locally and it all functions well. A couple of small comments on the validation and example help text.

@helenb Will push my latest code up as I haven't done that. It should be way better =) Might resolve some of the issues raised by you. Thanks Helen, will sort out any issues not addressed already locally and then push it up.

…ate one main menu instance in the wagtail admin. Migration file added.
…ks.base to be reusable across the project. LinkBlock from core.blocks.base is being used instead of the abstract class originally defined in models.py.
…enuViewSet added to restrict creating more than one main menu instance
@sanjeevz3009 sanjeevz3009 changed the title Main Menu - Back-end Main Menu Dec 9, 2024
@helenb
Copy link
Contributor

helenb commented Dec 9, 2024

I don't think the menu is using the styling quite as expected - compare my test with the menu settings (first screenshot) with what is in the prototype (second screenshot). I'm happy to pick that up as a follow-up task.

Screenshot 2024-12-09 at 15 26 12 Screenshot 2024-12-09 at 15 27 37

@sanjeevz3009
Copy link
Contributor Author

sanjeevz3009 commented Dec 9, 2024

@helenb

Thanks for testing this. I fixed the link issues.

As previously for highlights when a user selects a Wagtail page it didn't turn it into a link. Now it does. Same for the topic page.

Also fixed some other edge cases I missed out on previously, so now all should be good on that side.

Let me know if there are any further issues.

Now adding main menu content and it being usable on the website menu should be all good.

Screenshot 2024-12-09 at 18 31 58

So now we are just left with front-end alignment issues.

  • Where if you have only 2 columns the alignment looks weird and goes out of place slightly compared to 3 columns.
  • If you have 3 sections and the 1st one has 5 topics and then the rest of the sections have 3 topics then the the section alignment for that column goes off as well.
  • So there are similar issues like the above.

Would be amazing if you could follow that up on a task. Happy to demo the front-end styling/ alignment issues to you.

Thanks =)

@helenb
Copy link
Contributor

helenb commented Jan 21, 2025

So now we are just left with front-end alignment issues.

  • Where if you have only 2 columns the alignment looks weird and goes out of place slightly compared to 3 columns.
  • If you have 3 sections and the 1st one has 5 topics and then the rest of the sections have 3 topics then the the section alignment for that column goes off as well.
  • So there are similar issues like the above.

Would be amazing if you could follow that up on a task. Happy to demo the front-end styling/ alignment issues to you.

Thanks =)

Noting for completeness that these issues are addressed in #63

@sanjeevz3009
Copy link
Contributor Author

I've just done a functional test, and my only comment is that it would be better to just allow one set of main menu settings, and not have to manually select the menu once you've added it.

  • Initially I get the option to add a main menu, and this opens in a new tab which is confusing
  • After I save my main menu settings, I still have another step to select the main menu before it appears on the site
  • After I save the main menu, there is an option to choose another main menu, but the only option is the one I've already created, and there no way to create one
  • I can edit my main menu but not delete it in order to add a new one

I think it would be better if the settings -> navigation landed you straight in a screen with the main menu interface visible.

Update: after discussing this with Sanjeev I gather there are plans to improve this in future and Mebin has said it is OK for now.

@helenb Thank you Helen for this. We did have a good discussion about this. Yes, we will be reiterating this in the future especially when we get to Wagtail workflows etc. Additionally, as we discussed there are more questions that need to be answered and decisions that need to be made around the main menu features from having multiple main menus or having a single one, if we do maybe make it so we don't have to choose that in the navigation settings like you mentioned and also this is something @AdamHawtin pointed out and something I had in my mind. In most of these cases, @MebinAbraham and I have discussed these caveats.

I guess for now this is good as a first iteration, as we already scope creeped quite a bit in some areas.

I have made some notes of the feedback from you and @AdamHawtin. I will have some discussions with @MebinAbraham in capturing some of this somewhere or creating cards etc if needed soon.

Thanks!

@sanjeevz3009
Copy link
Contributor Author

So now we are just left with front-end alignment issues.

  • Where if you have only 2 columns the alignment looks weird and goes out of place slightly compared to 3 columns.
  • If you have 3 sections and the 1st one has 5 topics and then the rest of the sections have 3 topics then the the section alignment for that column goes off as well.
  • So there are similar issues like the above.

Would be amazing if you could follow that up on a task. Happy to demo the front-end styling/ alignment issues to you.
Thanks =)

Noting for completeness that these issues are addressed in #63

Thanks, Helen, tested this out and it works great, when I remember to do npm build on the branch I have your branch pulled into =)

@nehakerung nehakerung self-requested a review January 22, 2025 12:58
Copy link
Contributor

@nehakerung nehakerung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Did a thorough functional test and all looks well

Copy link
Contributor

@AdamHawtin AdamHawtin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the points @helenb raised, I found the workflow odd around being locked down to one main menu snippet, but then the settings menu still behaving as though there was a choice, but as a starting point to iterate on this, looks good, apart from small styling niggles noted the menu and editor options all work and look good.

It seems like the two rough paths we could go down to improve this are either:

  • Allow multiple different main menu snippets to be created, and allow users to then choose which is currently active. This would then allow users to either directly edit the active menu for small changes and/or author a new, separate, main menu snippets and switch over when they want them to become the active, published menu.
  • Stick to only allowing one main menu snippet which must be edited directly to make changes, but then also do the work to make this one main menu always active by default since it is the only choice.

We should probably also look at either removing the main menu drop down when no menu has been created, or populating some sensible defaults?

@zerolab
Copy link
Contributor

zerolab commented Jan 23, 2025

On having the navigation settings + menus as snippets:

  • the idea was to keep the settings clean, and allow editing the two menus separately. e.g. have different levels of access. This also make the UI cleaner in that you don't have to handle lots and lots of items all in one go
  • plus the original plan was to allow multiple main menu instances with one being in use, allowing for safe updates and prep ahead of time. This is now done via revisions.

@kacperpONS kacperpONS mentioned this pull request Jan 24, 2025
kacperpONS added a commit that referenced this pull request Jan 27, 2025
Copy link
Contributor

@RealOrangeOne RealOrangeOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved in the basis #85 and other future tickets will fix my outstanding comments.

@sanjeevz3009 sanjeevz3009 merged commit e921656 into main Jan 29, 2025
9 checks passed
@sanjeevz3009 sanjeevz3009 deleted the main-menu-CMS-169 branch January 29, 2025 10:17
sanjeevz3009 added a commit that referenced this pull request Jan 30, 2025
* Front-end hacked together to complete the back-end

* Navigation app added, models created for navigation settings

* Navigation bar on the front-end test

* Front-end code reverted due to the front-end main menu being done separately

* New line

* Code formatted

* Type hinted and code formatted

* Code refactored, abstract link class created, validation messages added, section restricted to 3, URL label added, icon for navigation settings added and restricting it to so only 1 main menu can be created

* Migration file

* Main menu modelling complete, restricting user to be able to only create one main menu instance in the wagtail admin. Migration file added.

* LinkBlock extracted out of core.blocks.related and moved to core.blocks.base to be reusable across the project. LinkBlock from core.blocks.base is being used instead of the abstract class originally defined in models.py.

* Code formatted, zombie code removed, unused imports removed and MainMenuViewSet added to restrict creating more than one main menu instance

* Front-end integrated

* Type hints added

* Nav template issues fixed

* gettext_lazy added

* Preview added

* Use the title provided by the user over the default title from the page

* help text update

* Type hints added

* Migration file regenerated

* Tests added

* TODO removed and translatable added

* Description help text updated

* Tests setup

* Example StreamField value setting in test_highlights_streamfield_limit

* Debug

* Errors being raised when adding more than 3 highlights

* Committing working factories to save

* Clean method for ColumnBlock added and tests updated

* Updated tests

* test_section_streamfield_limit test working

* Added more tests, factories updated

* Removed tests

* Max num code removed

* Code formatted

* Utilising pageurl to point to get internal wagtail pages

* Code refactored

* Reverting format for md files

* Reverting md files formatting

* Lint errors corrected

* Unused import removed and base template updated

* Base template update

* Type checking added back in

* Template code updated to make sure to only display live wagtail pages

* Main menu live preview fixed

* MegaLinter errors fixed

* Using a preview template for live previews

* Mark NavigationSettings as allowed for write in ExternalEnvRouter

* Use our own BaseSiteSetting which works with the ExternalEnvRouter

* RevisionMixin implementation and mypy fix

* Blocks refactor

* Code linted

* Tests for main menu viewset added

* Type ignore added

* Duplicate page and external URL validation for sections and topic links within columns complete.

* Duplicate page and URL for columns complete.

* test_forms in place

* Update test_clean_highlights_no_duplicate test

* Tests complete for forms.py

* test_forms.py code clean up

* Example usage zombie code removed

* Code formatted and refactored

* Code refactored, type hinted, tests refactored.

* gettext translation added

* LinkBlock Factory moved

* De-nest the blocks tests folder
sanjeevz3009 added a commit that referenced this pull request Feb 5, 2025
* Navigation templates tags added

* base template updated to use navigation template tags functionality, navigation_template_tags added and navigation_tags refactored.

* Main Menu (#41)

* _extract_highlight_item and _extract_url_item combined into one reusable function

* Type hinting fixed

* Using get_dummy_quest from wagtail for mocking requests

* Accidental files added, now removed

* Class CommonItem renamed to NavigationItem

* save method being called in test_navigation_tags
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 this pull request may close these issues.

None yet

8 participants