-
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
📈 [#2870] Add 'Mijn aanvragen' Siteimprove tracking (static) #1498
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1498 +/- ##
===========================================
+ Coverage 94.12% 94.23% +0.11%
===========================================
Files 1067 1073 +6
Lines 39338 39578 +240
===========================================
+ Hits 37026 37297 +271
+ Misses 2312 2281 -31 ☔ View full report in Codecov by Sentry. |
d4d3885
to
932a9e9
Compare
932a9e9
to
dc698a4
Compare
f95a565
to
e2b99b8
Compare
@pi-sigma @swrichards We do not have a Siteimprove test environment, so the only way to 'test' this code, is by locally uncommenting the Here is how a Siteimprove 'onClick' push event is constructed, and why it has consequences for their dashboard: https://help.siteimprove.com/support/solutions/articles/80000863895-getting-started-with-event-tracking#Setting-up-event-tracking The Siteimprove dashboard looks like this (screenshot in Anneke's comment from 15 Jan 2025): https://taiga.maykinmedia.nl/project/open-inwoner/us/2701 I have uploaded a new spreadsheet in Taiga (https://taiga.maykinmedia.nl/project/open-inwoner/task/2870) that shows which elements should be tracked, so if you click on any of those, you should be able to see a console log that indicates an event is pushed to the non-existing Or you could just do a review on code quality. |
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 I search for something, e.g. "yolo", I get the intended log message: Event pushed to _sz: Array(5) [ "event", "Header", "Zoeken", "Enter click", "yolo" ]
. So the mock _sz
object works. However, I don't see any logs when navigating to Mijn Aanvragen
via the menu.
That's odd @pi-sigma , but maybe your logs are cleared when you're (re)loading the page. |
Still no luck, same result (tried Firefox and Chrome with the preserver/persist logs option enabled). @swrichards does this work on your end? |
Same, I can see the events for e.g. entering a search query, but not for menu page navigations. See this dump (with persist logs), the click to "Mijn Vragen" is a few lines in, and I ended by typing something in the search box to show the logged event: |
'Open Mijn profiel mobiel', | ||
], | ||
// Desktop authenticated menu | ||
'.header > div > nav.primary-navigation.primary-navigation--desktop.primary-navigation__authenticated > ul > li > ul > li > a[aria-label="Mijn profiel"]': |
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.
@jiromaykin as far as I can tell, this selector does not select the menu items for me. I do not that the aria-label
on the final A
is Mijn Profiel
(note the casing).
More generally I was wondering: would it be an idea to move towards more targeted selectors? These feel quite fragile to me, they seem easy to break in a refactor (or even in an i18n change, as perhaps happened with the label), which wouldn't I think be covered by tests. We could maybe do something like:
<a href="/mijn-profiel/" class="link link--icon link--icon-position-before" aria-label="Mijn Profiel" title="Mijn Profiel" data-sz-id="mijn-profiel-link">
<span class="link__text">Mijn Profiel</span>
<span aria-hidden="true" class="material-icons-outlined ">apps</span>
</a>
And then use the data-sz-id
selector to more directly target the element, which hopefully in would be more resistant to other changes, as well as being a clearer marker during refactors of the HTML (and fewer elements to bind handlers to, which helps performance).
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.
@pi-sigma @swrichards Ah, I see.... I did not know people would use different menu titles (which they can indeed change in the CMS-admin as well) so it should work but only if wording/casing is 100% the same.
i can easily change it into a case-insensitive thing.
The data-attributes idea seems like a great idea, but we have a CMS menu in open_inwoner/templates/cms/menu/primary.html with link components that optionally already have a data-attribute (like so: "<li class="primary-navigation__list-item"> {% link text=child.get_menu_title href=url data_text=child.get_menu_title data_alt_text=child.get_menu_title data_icon=child.common.menu_icon data_alt_icon=child.common.menu_icon icon=child.common.menu_icon icon_outlined=True icon_position="before" %}"
which does not solve the javascript 'event bubbling' problem for all individual child elements, which could mean I'll have to add more listeners anyway.
Fragility will be less, but slightly unavoidable due to how specific Groningen needs the Siteimprove Dashboard to look like.
So I'm going to have to do quite some experimenting with this, and also not sure yet if this will make the DOM noticeably slower - since we have to use multiple 'pollers/MutationObservers', but it will probably be okay.
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.
that optionally already have a data-attribute
Quick question just on this point: why would that be an issue? HTML elements can validly have multiple data-*
tags right?
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.
Quick answer: no, that's not the problem, that's why, as you can see, they already have optional data attributes in the template tags. It's what you do with them that could be a potential problem. I meant: separate problem here is that data-attributes would not solve the fragility 100%.
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.
Understood, thanks!
63cbbf2
to
d56f90f
Compare
Issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2870
Hierbij zijn er verschillende statussen mogelijkIk wil weten hoeveel mensen een verkeerd bestand uploaden en daardoor een error krijgen. Dit uitgesplitst in een error van een te groot bestand en een error van een verkeerd bestandIk wil weten hoe vaak mensen een geupload bestand verwijderenThese last two-and-a-half events are more complicated and will be tackled separately in a different issue (#1563 ).