Merged multiple notifications under tracking code into one #20802
Conversation
|
I have concerns here around translations, I think composing the strings dynamically like we do here in the PR and only translating the word 'guide' could cause problems in other languages where the word guide could e.g. be in front of the subject of the guide. |
So what should we do here ? |
|
We shouldn't put together translations that way. Using a placeholder would be better then, or simply only displaying the name without appending Note: It actually even isn't correct to place names like |
|
@sgiehl @michalkleiner I have changed the translation for "guide", please check if this would work or not |
I think the updated approach still has the same problem, it adds the untranslated name of the service and translated word guide together, which still doesn't allow for word order change or adding special characters if needed etc. We can just leave it as Stefan suggested, i.e. remove the word guide from the actual guide link and let it just read as |
|
@AltamashShaikh #20818 has been merged so it'd be great if we can update this PR to use that and keep the old way for 4.x |
@michalkleiner Updated as per #20818, please check. |
| "VueDetectedEmail": "Vue.js was detected on your website. Did you know you can use the \"%1$s\" npm package to track the website data? Learn more here: %2$s", | ||
| "MergedNotificationLine1": "%1$s were detected on your website.", | ||
| "MergedNotificationLine2": "Learn more about configuring Matomo to work with these by following these guides: %1$s", | ||
| "ConsentManagerConnected": "We’ve detected that %1$s is correctly configured to work with Matomo on your site." |
There was a problem hiding this comment.
This was formerly PrivacyManager_ConsentManagerConnected in the template, should it stay in the PrivacyManager space?
There was a problem hiding this comment.
That would change the PrivacyManager AskingConsent code too, so preferred not to touch that
There was a problem hiding this comment.
This is all looking good to me, all the previous feedback has been implemented and relevant tests are passing. I did some additional manual checks with a few test sites setup with ga3/ga4 and various consent managers in different states of setup, the merged notifications handled them all correctly 👍
|
@bx80 / @michalkleiner can you merge this? |
* Merged multiple notifications under tracking code into one, #PG-2827 * Updated UI screenshot * design changes for notification bar * Updated UI screenshot * Improvements around translatio for guides * PR changes * Updated UI tests * Changes to incoprate consentManager connected * UI tests updated * Updated site_Type for test * Updated UI screenshot * Ampersand translated * Notifications logic updated * typo fixed * Twig varibale not defined fixed * UI tests fixed * UI screenshot updated * Started using createAndListing() instead of General_And * PR changes
* Merged multiple notifications under tracking code into one (#20802) * Merged multiple notifications under tracking code into one, #PG-2827 * Updated UI screenshot * design changes for notification bar * Updated UI screenshot * Improvements around translatio for guides * PR changes * Updated UI tests * Changes to incoprate consentManager connected * UI tests updated * Updated site_Type for test * Updated UI screenshot * Ampersand translated * Notifications logic updated * typo fixed * Twig varibale not defined fixed * UI tests fixed * UI screenshot updated * Started using createAndListing() instead of General_And * PR changes * Merged multiple notifications under tracking code into one, #PG-2827 * Update Controller.php removed unwanted use statements
Description:
Merged multiple notifications under tracking code into one.
Fixes: #PG-2827
Review