-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Get registration location map working with OSM #12495
Get registration location map working with OSM #12495
Conversation
59d936a
to
826d9eb
Compare
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's a nice addition, thanks.
I would like to see some test here, it's probably tricky to test the stimulus controller with Jest given the interaction with the map.But, I think we should at a minimum test that the OSM map is displayed when configured properly, which should be doable via system spec. Could you give it a go ?
No problem, thanks for the review. We could add a system test like below to https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/system/consumer/registration_spec.rb .... it "displays the location map using Open Street Maps" do
visit registration_path
switch_to_login_tab
fill_in "Email", with: user.email
fill_in "Password", with: user.password
click_button "Login"
click_button "Let's get started!"
# The .leaflet-container class is only present if map is initialised
expect(page).to have_selector("#open-street-map.leaflet-container")
end But then I added view and helper specs instead because I was wondering would a system spec abuse OSM tile servers by sending requests everytime the tests are ran. Not sure if that's okay? |
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 was wondering would a system spec abuse OSM tile servers by sending requests everytime the tests are ran. Not sure if that's okay?
Indeed that's an issue, and we don't want to rely on a third party when running system specs. There is always the option of stubbing the request to OSM/google map, but I like your idea of using a view test. System test are expensive to run and it's not strictly necessary for this.
Thanks for your help 🙏 !
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, thanks for implementing Leaflet/OSM!
I wondered if this new implementation could also be used for Google Maps (https://stackoverflow.com/questions/281264/remove-empty-elements-from-an-array-in-javascript).. but I think it's better to keep the official Google API.
if !$scope.latLong && openStreetMap && openStreetMap.dataset.latitude && openStreetMap.dataset.longitude | ||
$scope.latLong = { latitude: openStreetMap.dataset.latitude, longitude: openStreetMap.dataset.longitude } |
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.
FYI this could be a little neater with Optional chaining
(but it's totally fine as-is)
#displayMapWhenAtRegistrationDetailsStep() { | ||
const observer = new IntersectionObserver( | ||
([intersectionObserverEntry]) => { | ||
if(intersectionObserverEntry.target.offsetParent !== null) { | ||
this.#displayMap(); | ||
observer.disconnect() | ||
} | ||
}, | ||
{ threshold: [0] } | ||
); | ||
observer.observe(document.getElementById("registration-details")); | ||
} |
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.
Clever fix! I'm not sure if we'll need it for other purposes, but I think it could easily become a generic method (with the element ID as a parameter).
Hey @cillian , Thank you for the detailed instructions. Proceeded as you've suggested, and was able to verify that the Open Street map is displayed when creating an enterprise: Also, I've verified that the a change is persistent when one adjusts the suggested location (after typing in an address and clicking Locate on Map). Below, a change from the initially displayed position 1) from 2) was made (when creating an enterprise), which is then shown, when visiting the /map tab: (apologies, no video... one was recorded with Loom, but somehow it is still uploading, and I think it does not make sense to wait longer...) I've made a quick test to verify that no regression is introduced, i.e., that having the open street maps disabled, allows using the previous google-maps as before - all good 👍 Also, verified that enabling OpenStreetMaps with an invalid config does not break the page anymore: This looks great, merging this PR. Thank you 💪 |
Great job! |
What? Why?
This add two things to get a bit closer to #5542
(1) It prevents a broken map from appearing during registration if an instance has a
GOOGLE_MAPS_API_KEY
environment variable present i.e. they already have Google Maps enabled AND that instance then enables Open Street Map (OSM). This issue was spotted here. The fix for this is in theusing_google_maps?
method.(2) It gets the registration location map working with OSM, see:
osm-registration-map.mp4
What should we test?
Configuration > Content
section.Release notes
Changelog Category (reviewers may add a label for the release notes):