-
Notifications
You must be signed in to change notification settings - Fork 506
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
docs: change (broken) Bing maps example to heatmap in {pkgdown} site #934
base: main
Are you sure you want to change the base?
docs: change (broken) Bing maps example to heatmap in {pkgdown} site #934
Conversation
bing was broken; example taken from https://gist.github.com/jcheng5/c084a59717f18e947a17955007dc5f92
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.
Thanks for the PR @jack-davison! I have just a few small comments...
vignettes/articles/extending.Rmd
Outdated
# This tells htmlwidgets about our plugin name, version, and | ||
# where to find the script. (There's also a stylesheet argument | ||
# if the plugin comes with CSS files.) |
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.
These comments point out that the example is rather sparse and could benefit from description. Personally, I'd prefer to have these comments appear in prose rather than in comments.
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.
Sorry, I should have also mentioned that I don't feel particularly strongly about this. It's an obvious improvement; so if you have a stronger preference for the comments or don't have the time now I'd 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.
I think I'd agree they'd make more sense in the body of the document rather than the comments
(they're only there because it was adapted from a gist) - I've had an initial go at moving them out into the prose, although perhaps better for someone with more clear knowledge of {htmlwidgets}
to give it a crack!
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.
e.g., I'm not totally clear on what x
and el
represent in the onRender()
function - although I can see they're not used in this case.
Co-authored-by: Garrick Aden-Buie <[email protected]>
Co-authored-by: Garrick Aden-Buie <[email protected]>
Fixes #927
Briefly, this replaces the now-broken Bing 'extending Leaflet' example here with the heatmap one found at: https://gist.github.com/jcheng5/c084a59717f18e947a17955007dc5f92
Also removes a duplicate NEWs item.
Example now looks like this: