-
Notifications
You must be signed in to change notification settings - Fork 6
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
💄 [#1067] Mobile menu-structure with submenu toggle #506
💄 [#1067] Mobile menu-structure with submenu toggle #506
Conversation
794788c
to
415657a
Compare
…re-with-submenu-toggle' into feature/1067-mobile-menu-structure-with-submenu-toggle
@vaszig and @bart-maykin : don't be afraid to review this - the best way to do this, is to checkout the branch and actually view it in the browser in moblie views; (just looking at all the code might be a bit too much for humans :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing it on the browser looks good. Generally I think it would be easier and better to have smaller PR's (here we combine 14 tasks) when we have to do with changes that may affect a lot of css and html files (It has to do with one task, indeed, but I think we should split this up in the future).
@@ -33,7 +33,8 @@ | |||
</head> | |||
|
|||
<body hx-headers='{"X-CSRFToken": "{{ csrf_token }}"}'> | |||
{% header categories=menu_categories request=request breadcrumbs=breadcrumbs search_form=search_form has_general_faq_questions=has_general_faq_questions %} | |||
{% get_solo 'configurations.SiteConfiguration' as config %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary, we already load the utils so we have access to the context_processors (like we do above with the colors and below with the has_general_faq_questions
). So, you can add show_plans=show_plans
to the header template tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, @vaszig is correct. Note this get_solo was both my suggestion (because we looked at the other code that did something like that), and I even added the line to the context processor earlier:
"show_plans": config.show_plans, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did it correctly now, but keeping this comment for future reference.
src/open_inwoner/components/templates/components/Header/Header.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/open_inwoner/components/templates/components/Header/Header.html
Outdated
Show resolved
Hide resolved
{% if request.user.get_new_messages_total %} | ||
{% with "("|addstr:request.user.get_new_messages_total|addstr:")" as message_total %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty impressive filter use 😄
The request.user.get_new_messages_total
is doing queries though, so it is better to put its result into a variable and re-use it, instead of querying the same thing twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bartvaderkin I copied all of the already existing code from the PrimaryNavigation template to the Header template, so I did not come up with these myself; this will be a learning experience for me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the help of Vasileios I added {% with request.user.get_new_messages_total as total_messages %}
in both header and Primary navigation.
Oh dear... @Bartvaderkin, I will think of a way to solve this for someone named Wolfeschlegelsteinhausenbergerdorff-Senior and will probably use "get_short_name" for mobile view + I'll probably talk to the designer. |
Codecov Report
@@ Coverage Diff @@
## develop #506 +/- ##
===========================================
- Coverage 96.53% 96.52% -0.02%
===========================================
Files 521 527 +6
Lines 18781 19023 +242
===========================================
+ Hits 18131 18361 +230
- Misses 650 662 +12
... and 27 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Lets not let perfect be the enemy of good, cutting off very long names on mobile would be acceptable in my eyes |
OK, I will add a cut off - I only just found out something like & Today the mobile design was updated, which shows the text border needs to be removed above the search bar: https://projects.invisionapp.com/share/3E135L29X8BP#/screens/471881064 |
This PR can only be reviewed properly when checked-out and run in the browser.
See issue https://taiga.maykinmedia.nl/project/open-inwoner/task/1067
list-style: none;
)