-
-
Notifications
You must be signed in to change notification settings - Fork 214
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
Added zoom to location fields #1835
Conversation
Right now location fields always load at the default zoom level of 15 and there's no way to customize it. I've added an extra attribute to the location field so you can override the zoom level in the resource. I've also made mapkick show the map controls so users can zoom in and out if they want to.
Code Climate has analyzed commit 9943d61 and detected 0 issues on this pull request. View more on Code Climate. |
See avo-hq/avo#1835 for details
I've added documentation for this in avo-hq/docs.avohq.io#66 |
|
||
def initialize(id, **args, &block) | ||
hide_on :index | ||
super(id, **args, &block) | ||
|
||
@stored_as = args[:stored_as].present? ? args[:stored_as] : nil # You can pass it an array of db columns [:latitude, :longitude] | ||
@zoom = args[:zoom].present? ? args[:zoom].to_i : 15 |
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 just saw on the docs that @zoom
accept values between [0, 22] I think that would be interesting to raise an informative error about the accepted range if for some reason the value goes out of this range. @iainbeeston and @adrianthedev what do you think?
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.
Yes that's a good idea... but what is the best way to do that? Should it raise an error if that happens?
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.
My first thought was "let's just take the argument from the user and if it's greater than 22, then we force it as 22, but that would generate some technical support as "why isn't Avo working and not letting me add a number greater than 22".
The second thought I had was, to raise an error when that number is greater than 22. But what if the API changes at some point? We'd have to maintain that parity between our validation and the external API.
Now, I'm thinking of just leaving it without validation and directing the user towards the original library (mapkick) because of those two reasons.
So my vote goes to leaving it like that and instructing the user that we only forward the options to the original library.
Thank you for the amazing work @iainbeeston! |
Climbing the ladder @iainbeeston |
Description
I've added an extra attribute to the location field so you can override the zoom level in the resource. I've also made mapkick show the map controls so users can zoom in and out if they want to.
Fixes # (issue)
Right now location fields always load at the default zoom level of 15 and there's no way to customize it.
Checklist:
Manual review steps
zoom: 1
on the location fieldManual reviewer: please leave a comment with output from the test if that's the case.