-
Notifications
You must be signed in to change notification settings - Fork 351
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
Canny Support #1282
Canny Support #1282
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.
Great to see the increase of proper types :party:
'https://neo4j-browser.canny.io/feature-requests' | ||
|
||
declare global { | ||
interface Window { |
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.
second global window declared in this PR, are they automatically merged? or are they only global for this 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.
Typescript interfaces are merge when declare several yes, we could move this declaration to the custom file, not sure what is more structured
const e = document.createElement('script') as HTMLScriptElement | ||
e.type = 'text/javascript' | ||
e.async = true | ||
e.src = 'https://canny.io/sdk.js' |
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.
Now this feels pretty cursed for some reason, I guess it's not much different to a normal cdn:ed dependency
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.
Any suggestions on how to dispel the curse?
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'll have to live with it if we want canny 🤷
This PR contains 5 changes:
The changes can be demoed in this sandbox: http://canny.surge.sh/