💄 [#1492] Adding notification dot in login menu #739
Conversation
jiromaykin
force-pushed
the
feature/1492-display-notification-dot
branch
2 times, most recently
from
34445d5
to
97c75a6
Compare
jiromaykin
force-pushed
the
feature/1492-display-notification-dot
branch
from
26731b0
to
806f62b
Compare
| {% for child in children %} | ||
| {% if child.indicator %} | ||
| <span class="indicator"><span class="indicator__dot"></span></span> | ||
| {% endif %} | ||
| {% endfor %} |
There was a problem hiding this comment.
This for-loop is probably not what we want, as there could be multiple menu items with indicators.
Also there is a weird situation because the children context variable shouldn't be present at this point, it supposedly gets generated by show_menu_below_id a little further down.
I'll investigate what is going on and create a snippet of code for this.
There was a problem hiding this comment.
This seems to be a pattern. Cf. e.g. templates/cms/menu/primary.html: a loop over children, even though the context is updated by show_menu further down.
There was a problem hiding this comment.
It seems like a bug or flaw in django-cms where it doesn't use the context stack the way template tags are supposed to. I filled an issue django-cms/django-cms#7628 but I don't expect anything from it.
jiromaykin
force-pushed
the
feature/1492-display-notification-dot
branch
2 times, most recently
from
d6b82b8
to
7eae139
Compare
jiromaykin
force-pushed
the
feature/1492-display-notification-dot
branch
from
7e870c3
to
7b6cc04
Compare
…menu-indicators issue & context pollution Also dropped the `header` template tag.
…ation-dot-fix [#1492] Capture django-cms menu output and re-use, as workaround for menu-indicators issue & context pollution
|
@jiromaykin We were a bit too early to merge my fix branch, but at least the test did catch the issue you spotted as well. I appended a fix, let's see if it turns green. |
| @@ -86,7 +86,7 @@ def test_only_published_subcategories_exist_in_detail_page_when_logged_in(self): | |||
| def test_only_published_categories_exist_in_my_categories_page(self): | |||
| response = self.app.get(reverse("profile:categories"), user=self.user) | |||
| self.assertEqual( | |||
| list(response.context["categories"]), | |||
| list(response.context["menu_categories"]), | |||
There was a problem hiding this comment.
This test didn't test what it supposed to test (it is not about the menu but about a form), so this fix is not what we want. I'm rewriting it right now.
There was a problem hiding this comment.
@jiromaykin I pushed a fix for this as well.
There was a problem hiding this comment.
@Bartvaderkin The bad test is no longer bad - all seems good now! + and @pi-sigma also the "configure Onderwerpen to become invisible for logged out users" still seems to be working this way. May have something to do with the 'context'.
Codecov Report
@@ Coverage Diff @@
## develop #739 +/- ##
========================================
Coverage 96.28% 96.28%
========================================
Files 676 676
Lines 24044 24044
========================================
Hits 23151 23151
Misses 893 893
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
https://taiga.maykinmedia.nl/project/open-inwoner/task/1492
show just a dot IF there are any notifications for messages OR plans.
we do not have a design for this so I just made it look like this (dot has the same size as the other one and overlaps the down-arrow a bit):