RefugeRestrooms / refugerestrooms

REFUGE restrooms indexes and maps safe restroom locations for trans, intersex, and gender nonconforming individuals.
http://www.refugerestrooms.org
GNU Affero General Public License v3.0
888 stars 262 forks source link

Applying ADA filters for the Map view of search. #651

Closed GPrimola closed 3 years ago

GPrimola commented 3 years ago

Context

Summary of Changes

Checklist

Screenshots

Before

Screen Shot 2020-10-18 at 16 41 21 Screen Shot 2020-10-18 at 16 43 03 Screen Shot 2020-10-18 at 16 43 12

After

Screen Shot 2020-10-18 at 16 28 07 Screen Shot 2020-10-18 at 16 28 28 Screen Shot 2020-10-18 at 16 28 46 Screen Shot 2020-10-18 at 16 28 59
DeeDeeG commented 3 years ago

Looks good, thanks!

I'll take a closer look at the code, but mostly I'll be relying on the tests and a manual functional check using either Docker or Heroku.

Would be great to have this all working together.

DeeDeeG commented 3 years ago

Good news, this has no merge conflicts with develop branch after #644 was merged. And there are no Rubocop linting warnings, as this PR is almost 100% JavaScript changes.

I'm also happy to report this passes our automated testing (Travis CI).

I'll run this through some manual functional tests, and if all looks good, that's the last thing to "check off" before merging. Thanks for the contribution, once again.

Testing as hosted here: https://refuge-deedeeg-test.herokuapp.com/

DeeDeeG commented 3 years ago

Hi @GPrimola,

I noticed in my testing that the info bubbles are popping up blank on this branch.

This branch (https://refuge-deedeeg-test.herokuapp.com/):

Restrooms map view with blank restroom info popup over a map pin

develop branch (https://staging.refugerestrooms.org):

Restrooms map view with populated restroom info popup over a map pin

Are you able to look into this and see if there is a way to make the popups show their content again?

Thanks.

- DeeDeeG

GPrimola commented 3 years ago

Sure @DeeDeeG !!

Sorry to ask, but do you think this is related to this changes? Because thinking very shallow here, it doesn't make much sense this changes would introduce this kind of behavior.

But sure I can check what's going on!

DeeDeeG commented 3 years ago

@GPrimola that is a fair question.

I just double-checked in case I was mistaken (it happens). I have an instance of Heroku for this branch exactly: https://refuge-deedeeg-test-2.herokuapp.com, and this branch with the Rubocop changes (the tip of the develop branch) merged in: https://refuge-deedeeg-test.herokuapp.com.

Both instances have the blank info popups.

And develop branch is running here: https://staging.refugerestrooms.org/ and here: https://refuge-deedeeg-develop.herokuapp.com with working map pin popups.

The only difference is the two commits on this branch. So I am fairly confident the changes in this branch are causing the different behavior on the site.

GPrimola commented 3 years ago

Perfect @DeeDeeG !! I'll check it this weekend and push the changes by monday!

GPrimola commented 3 years ago

@DeeDeeG I just pushed some changes that fixed the problem. Can you check if it's ok?

Thank you!

DeeDeeG commented 3 years ago

@GPrimola It's working, thanks very much for posting and addressing review comments on both of these pull requests.

DeeDeeG commented 3 years ago

@GPrimola If you are interested, I think it would be nice to make it so that switching from list view to map view and back doesn't require a page reload. (It makes some sense that changing the filters could require a new page load, due to needing a new API call to get the new set of restrooms matching the new filters. But once we have a set of restrooms on a page, I believe we have enough data loaded to switch from map view to list view and back.)

But this pull request is already quite good as it is, and that would be extra if you decide to take it on. And I completely understand if you don't want to. It already works well the way it is.

For now, I'll merge this the way it is. Thanks again.