Closed piercebb closed 4 years ago
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.
Context
Summary of Changes
infoWindow
when the map is selectedinfoWindow
infoWindow
when another marker (bathroom) is selectedChecklist
Screenshots
Before
After