Skip to content
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

make sure drawing happens in a sane place #79

Open
tim-salabim opened this issue Jul 29, 2018 · 5 comments
Open

make sure drawing happens in a sane place #79

tim-salabim opened this issue Jul 29, 2018 · 5 comments

Comments

@tim-salabim
Copy link
Member

See https://stackoverflow.com/questions/51580551/convert-xy-to-lat-long-in-r

We should probably add a check whether drawing happened / is happening within acceptable longlat bounds (-180/180 & -90/90)

@tim-salabim
Copy link
Member Author

@timelyportfolio an attempt at signaling when features are drawn in a location outside the standard longitude range between -180 & 180. This now emits a warning which is possibly highlighted using crayon (if available) in nasty red background with white text :-) .
In my opinion, this is sufficient, but we could make sure to mention somewhere that this warning is generated if features are drawn outside sane bounds (which, for whatever reason, is allowed by leaflet).
Any other suggestions? Thoughts?

@mdsumner
Copy link
Member

mdsumner commented Aug 2, 2018

Very happy it's just a signal and not an error :)

@timelyportfolio
Copy link
Contributor

timelyportfolio commented Aug 3, 2018

@tim-salabim would this require a dependency on crayon? Are you thinking an error from JavaScript or R? Assuming R, and in this case we could add a test when we mutate the state based on Leaflet.draw events. Or would you rather delay the test until completion?

@tim-salabim
Copy link
Member Author

@timelyportfolio crayon is in Suggests: with a if (requireNamespace("crayon")) conditional on the warning. In my experience, I pay even less attention to console output in shiny apps than in interactive mode, that's why I thought highlighting is helpful in this case.
With the above commit, this warning is emitted every time (part of) a feature is drawn outside -180 / 180 longitude. I think this is what you mean with "a test when we mutate the state based on Leaflet.draw events", right? If not, I think I need more clarification.
Regarding a JS warning, I think it would be great as it would provide more direct feedback especially when working in the browser. Though, I would only do this if we can assure that it doesn't interrupt interaction too much (people may intentionally want to draw outside standard bounds, who knows). I first thought of a alert call, but this would be really annoying I think. If we can find a good place in the window/map to place such a warning, then I would say it is desirable to emit directly via JS (potentially keeping the R warnings as well). But this is something that I would not know how/where to implement.

@timelyportfolio
Copy link
Contributor

@tim-salabim I see now. I missed your commit and misunderstood thinking we were discussing in theory. Adding a JavaScript warning should be fairly straightfoward. I will leave this open as a reminder to work on this once we have finished the prioritized open items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants