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

[#1378] Updated new menus to use dynamic pages #613

Merged
merged 3 commits into from
May 13, 2023

Conversation

Bartvaderkin
Copy link
Contributor

@Bartvaderkin Bartvaderkin commented May 11, 2023

This assumes you have the authenticated pages as children of the homepage in the tree, and use 'home' as ID in the page's advanced settings.

The products/onderwerpen are still rendered as they were, until we move the content to actual pages.

Note I also added a context manager that adds some data to check if an app is active.

Relates to #591

@jiromaykin jiromaykin force-pushed the feature/1378-mobile-header-navigation-no-hamburger branch from c80f731 to 9ddb4d1 Compare May 11, 2023 11:06
@Bartvaderkin Bartvaderkin marked this pull request as ready for review May 11, 2023 12:11
@alextreme alextreme requested a review from jiromaykin May 11, 2023 12:47
@jiromaykin jiromaykin force-pushed the feature/1378-mobile-header-navigation-no-hamburger branch from 9ddb4d1 to 6f4e15d Compare May 12, 2023 09:15
Copy link
Contributor

@jiromaykin jiromaykin left a comment

Choose a reason for hiding this comment

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

Seems to work great!
I assume the icons before the pages will be added later on as an option?
Or is that already active somewhere?

Screenshot 2023-05-12 at 11 23 00 Screenshot 2023-05-12 at 11 23 45

@jiromaykin
Copy link
Contributor

@alextreme and @Bartvaderkin I rebased my own branch from PR591, which maybe resulted in conflicts in this branch?
I hope this branch can be approved so everything can be merged and I can clean up the menu and remove all unnecessary comments in Pull Request 591

Base automatically changed from feature/1378-mobile-header-navigation-no-hamburger to develop May 12, 2023 13:49
@codecov-commenter
Copy link

Codecov Report

Merging #613 (458dc2f) into develop (50768aa) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #613      +/-   ##
===========================================
- Coverage    96.52%   96.40%   -0.12%     
===========================================
  Files          577      579       +2     
  Lines        19891    19942      +51     
===========================================
+ Hits         19199    19226      +27     
- Misses         692      716      +24     
Impacted Files Coverage Δ
src/open_inwoner/conf/base.py 95.37% <ø> (ø)
src/open_inwoner/cms/context_processors.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@alextreme alextreme left a comment

Choose a reason for hiding this comment

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

Could use some caching to avoid determining the active apphooks on every request, but considering it's a single query this should be okay for now

@alextreme alextreme merged commit f211001 into develop May 13, 2023
@alextreme alextreme deleted the feature/1378-dynamic-menu branch May 13, 2023 21:50
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.

4 participants