-
-
Notifications
You must be signed in to change notification settings - Fork 163
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
Matomo Analytics Support #112
Conversation
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 we're missing some additional config for ENV variables that you had in your last PR? We need to always configure the RAZZLE_
ones too! See UMAMI_WEBSITE_ID
or GA_TRACKING_ID
in server.js
These env's are the same as for the other script, so these have already been initialized. Or am I not getting something here? I don't understand |
If they are not configured there we might not have access to them while debugging locally, although this is really only needed server side it's still nice to be consistent. |
Isn't what this is for?
Also, I saw the commit had been applied from #111 yet I cannot see the changes in the files. |
No problem! Yes, that's the change but it's in a PR that you closed. I don't see those changes in this PR listed here https://github.com/techno-tim/littlelink-server/pull/112/files You are so close, just open up your current PR and add those changes and push it up! |
I didn't mean to do that! I had a problem where my first repository committed every file under my comment "matomo support" and therefore I deleted it and created a new one. Didn't know that would close #111 since I thought that was already pushed. I think I have now added it again, do we have everything now? :) |
this closes #41 |
Thank you! Will review in the morning! |
Thank you for bearing with me :) |
src/server.js
Outdated
runtimeConfig.MATOMO_URL && runtimeConfig.MATOMO_SITE_ID | ||
? ` | ||
<!-- Matomo Image Tracker--> | ||
<img referrerpolicy="no-referrer-when-downgrade" src="${runtimeConfig.MATOMO_URL}matomo.php?idsite=${runtimeConfig.MATOMO_SITE_ID}&rec=1" style="border:0" alt="" /> |
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.
one small thing here! I think this would be
<img referrerpolicy="no-referrer-when-downgrade" src="${runtimeConfig.MATOMO_URL}/matomo.php?idsite=${runtimeConfig.MATOMO_SITE_ID}&rec=1" style="border:0" alt="" />
(missing /
between the site and matomo.php?
) Otherwise your url would have to end in a trailing slash, which might be fine but also odd that you would have to know to end your site in a slash. What do you think?
after this, looks good!
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.
The reason I did this is because the slash is there as well in the other part of the config where it just asks for the url. I didn't test if the slash was necessary there. Second thing I was thinking is that they will probably check out their config with this same code cause don't know how else you can get the site ID. I was thinking if they copied the URL from there (which has trailing slash) it would then create errors.
I also thought the same like you that it's pretty confusing. I guess it's confusing depending from which point you view it and could be either way confusing.
I'll add another commit to what you said. Maybe my logic is too far fetched.
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 see what you mean but I think this is cleaner for the person using the URL as a variable, keep the ugly stuff in code ;) Thank you!
You're doing fine! One small question / possible fix, after that looks good! |
No trailing slash needed in the variable "MATOMO_URL"
Fix has been committed :D |
Thank you for you contribution! |
Proposed Changes
In addition to Matomo Support #111 to track browsers with javascript disabled
Checklist
yarn ci
to test my code