-
Notifications
You must be signed in to change notification settings - Fork 261
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
Add listeners to close infowindow when another is opened #628
Conversation
The code change looks very straightforward, easy to review. CI passed as well. I'd like to poke around with this in Docker on my machine and just give it time in case something jumps out as needing a change, but otherwise, I don't see why not to merge this. (@stardust66 you are pretty good with javascript! If you happen to want to take a look at this, your feedback would be appreciated. It's a very small change though, so don't feel bad if you are busy. I can test it myself, but multiple reviewers is always a great thing to strive for IMO.) Once again, thank you for the contribution @piercebb, and thank you for filling out the PR template so fully. It really illustrates what is going on with this change. ❤️ Very easy to review. Tests: If it was a bigger change (and in general) it's good to have tests for every little feature, but this is more of a "polish/nice-to-have" thing and it's okay without a test. If you are up for it, technically we do like to have tests for as much of the codebase as possible. It would be something like "When I click on the map, an already-open restroom info popup is closed". And simulate a click somewhere on the map with Capybara. We have some docs about that in our Wiki. I am comfortable in this case with not requiring a test, because I think the test would be harder to code than the feature itself, and as mentioned above, it's a nice-to-have feature, not a core feature. Let me know if you are working on a test and then I will leave this open to make sure you have time to get that working. If I don't hear back about that, it's fine and I'll move ahead without expecting a test to be added for this. Truly fine either way. Could be a good learning experience for some of our contributors, so that's a reason I suggest it as well. |
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.
This looks good to me!
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.
Looks totally good on my machine in Docker as well.
Thanks @stardust66 for your review.
Ready to merge this!
* Add reference to docker troubleshooting wiki in `CONTRIBUTING.md` (#625) - Add reference to docker troubleshooting wiki in contributing documentation - Update CONTRIBUTING.md * Add listeners to close infowindow when another is opened (#628) * CONTRIBUTING.md: Docker for Windows requirements - Docker Desktop for Window requires Windows 10 64-bit, "Professional" or "Enterprise" editions. * Update dependencies June 2020 (#629) - dependencies: Bump geocoder, websocket-extensions - Gemfile.lock: Update rack to 2.2.3 Co-authored-by: Pierce Bala <65255475+piercebb@users.noreply.github.com>
Context
Summary of Changes
infoWindow
when the map is selectedinfoWindow
infoWindow
when another marker (bathroom) is selectedChecklist
Screenshots
Before
After