-
Notifications
You must be signed in to change notification settings - Fork 2
Connect User Metrics #181
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
base: main
Are you sure you want to change the base?
Connect User Metrics #181
Conversation
…rics-prepare-manifest-files
add connect-user-metrics dir
update links with shortener
Add About the app section
Pre-release manifest.json update.
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.
Howdy @jakubnowicki 👋
Thanks for submitting this PR to contribute to the Connect Gallery! I'm still working through this PR to give it a full review, but I figured I'd post my in progress comments and questions so we could start some back and forth while I work through it.
@@ -0,0 +1,55 @@ | |||
# Connect Insights Dashboard |
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.
Great README. I noticed you included a short description in the manifest.json
, but it could be helpful to add a description here as well. Perhaps one even longer that gives an indication of what functionality to expect.
Additionally I see the "Connect Insights Dashboard" naming here, but the directory and manifest.json
use "Connect User Metrics" I was curious about the difference in naming.
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.
Excellent idea! I will copy the content of the in-app "help button" here, so we have a better description of what the app really does.
About the naming convention: you are right, we will stick to "Connect User Metrics".
Confirm with your Posit Connect admin that instrumentation is enabled. | ||
|
||
<!-- Links --> | ||
[User Guide Vars]: https://docs.posit.com/connect/user/content-settings/#content-vars |
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.
Looks like there is a typo here.
[User Guide Vars]: https://docs.posit.com/connect/user/content-settings/#content-vars | |
[User Guide Vars]: https://docs.posit.co/connect/user/content-settings/#content-vars |
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.
Nice catch!
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 noticed there are some features not included in the README
that are in the Appsilon/ConnectUsersMetrics
repo README. That totally makes sense given how the READMEs will be consumed for Gallery content.
It makes me wonder if there should be a CONTRIBUTING guide here that includes some of those details so this file, for example, is a bit easier to understand.
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 app here is a simplified version of the origina Connect User Metrics; some of the features only make sense in the complete app, where the user is able to modify some configuration files.
I will add a CONTRIBUTE file pointing to the original branch.
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.
It looks like the js
file is empty other than this one file. Is there a reason to keep it around that I'm missing?
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.
You are right! These files (together with Cypress testing) are automatically generated Rhino, the framework we used to make this app. I will delete it for the sake of simplicity.
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.
It looks like the Cypress tests aren't used here and there isn't a package.json
to setup Cypress with npm
. Can we remove the Cypress config and this empty test file?
When Cypress tests are added in the future we can get them added back :)
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.
(Same answer as above; I will delete the file).
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.
It looks like this is being generated from main.scss.
but I'm curious how this is being generated. I don't see anything mentioned in the README here or over in https://github.com/Appsilon/ConnectUserMetrics
Forgive me this may be some of my own unfamiliarity with styling Shiny apps.
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 minified CSS is generated by running rhino::build_sass()
; it is a common practice when using Rhino, but we probably can make that more clear in the main repository of Connect User Metrics.
…-fixes' into pre-connect-release-fixes
Hey, @dotNomad , thank for the quick review! Jakub is on vacation right now, so I will try to answer some of your comments. I made a PR-to-this-branch fixing the issues you pointed. As soon as Marcin approves it, we can make a new round of reviews. |
Copy changes from main-connect-extension
Dashboard for presenting the adoption data for the content deployed on Posit Connect, created by Appsilon.
Standalone repository: https://github.com/Appsilon/ConnectUserMetrics