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

GPrimola · 2020-10-18T19:45:03Z

Context

Fixes Filter buttons (ADA/inclusive) do not apply to Map View #479
With this PR, the filters will persist during the change of the view from list view to map view. This is being done through query parameters to the controller.

Summary of Changes

There's a new instance attribute called @view holding one of list or map values.
The index.html.haml had to be changed to show the list view or the map view of the search restrooms.
The index.js was changed with a new logic to support the new behavior when loading the map.

Checklist

Tested Mobile Responsiveness
Added Unit Tests
CI Passes
Deploys to Heroku on test Correctly (Maintainers will handle)
Added Documentation (Service and Code when required)

Screenshots

Before

After

DeeDeeG · 2020-10-20T01:03:55Z

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 · 2020-10-20T01:05:14Z

app/javascript/packs/views/restrooms/index.js

-      list.fadeOut(500, function() { mapContainer.fadeIn(500, initPoints) });
-    }
-    mapShow = !mapShow;
+// Polyfill for Poltergeist.


We do hope to support most older browsers. As long as it's reasonably easy to do so.

I guess Poltergeist is the one that needs to work for testing purposes... but this comment can probably say "Polyfill for older browsers"? As we don't have Poltergeist when running in Heroku/production/the real website.

Yes @DeeDeeG you're right!
I'll change this to old browsers as it fits better!

Thanks!

An update on the review process: I'm hoping to get the rubocop pull request (#644) merged first thing, then come back to pull requests such as this one.

Sure @DeeDeeG ! No rush!

DeeDeeG · 2020-10-23T17:07:15Z

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 · 2020-10-23T17:44:54Z

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/):

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

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 · 2020-10-24T16:15:42Z

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 · 2020-10-24T17:05:34Z

@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 · 2020-10-24T17:14:06Z

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

GPrimola · 2020-10-25T16:46:51Z

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

Thank you!

DeeDeeG · 2020-10-26T15:49:32Z

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

DeeDeeG · 2020-10-26T15:54:28Z

@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.

* Finish Upgrading Webpacker to v5 (#637) - Gemfile[.lock]: Update webpacker to 5.2.1 - Webpacker: Update config files Ran "rails webpacker:install" and committed the changes. In "config/webpacker.yml," kept the ".js.erb" entry under "extensions:" so that "app/javascript/packs/lib/maps.js.erb" is included. In "config/webpack/environment.js," kept jQuery and rails-erb-loader configs, as we are still using these. - JS dependencies: Upgrade webpack-dev-server Commit the changes from running `rails webpacker:install`, but using a caret "^" semver range for "@rails/webpacker", rather than an exact version. (So that we get updates in the 5.x series) * Update dependencies for September 2020 (#636) - dependencies: Remove `swagger` node module We don't actually use this. Also removes 200 sub-dependencies, and makes our yarn.lock file about 1270 lines shorter! - yarn.lock: Upgrade bootstrap to 4.5.2 (Was at version 4.4.1) - dependencies: Resolve node-fetch to ^2.6.1 - yarn.lock: Upgrade "selfsigned" and "node-forge" - Gemfile[.lock]: Update Rails and dependencies rails was 5.2.4.3, now it's version 5.2.4.4 * Keeping filters state on pagination (#638) - Keeping filters state on pagination - Fix Code Climates complaints. - Fixed active issue on ada filter buttons - Fixed undefined function problem when run rspec. * Add rubocop and resolve lint errors (#644) - Initialize rubocop - Run rubocop with --fix - Don't require magic comment This will affect every file and have potential side effects, so I'm going to start with this turned off. - Resolve lint errors in `app` - RSpec linting - Resolve remaining - Fix wrong change - Singleton method for verify - New lint rules - Add rubocop to travis config - Correct docker compose command - Check for geo The condition was actually supposed to be `if geo = results.first`, because that's not always obvious the intention and fails lint, I'm just doing a truthy check for it which should be the same. - Fix typo - Add contributing docs for rubocop * Applying ADA filters for the Map view of search. (#651) - Applying ADA filters for the Map view of search. - Changed comment for polyfill URLSearchParams - Fixed bug of map marker content not showing. * Add a standard EditorConfig configuration for every contributor to follow (#649) * Enhancement/i18n thread safe (#647) - Provide a way to respond requests without thread-safe issues As rails docs says https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests When we use 'I18n.locale =' the current locale could leak into following requests this is a standard and recommended way to deal with it :) - Create a shared_example to test I18n locale switching - Update spec/controllers/pages_controller_spec.rb * Upgrade puma to latest version 5.x (#641) * yarn.lock: Update "http-proxy" to v1.18.1 (fe195d7) Co-authored-by: Lucas <torres.giorgio@gmail.com> Co-authored-by: Tegan Rauh <3896310+tegandbiscuits@users.noreply.github.com> Co-authored-by: Bruno Casali <brunoocasali@gmail.com>

Applying ADA filters for the Map view of search.

780281b

DeeDeeG reviewed Oct 20, 2020

View reviewed changes

DeeDeeG added Hacktoberfest These are issues or pull requests related to Hacktoberfest (https://hacktoberfest.digitalocean.com/) Ready for Review labels Oct 20, 2020

Changed comment for polyfill URLSearchParams

b990e69

DeeDeeG added the enhancement label Oct 23, 2020

Fixed bug of map marker content not showing.

9546234

DeeDeeG approved these changes Oct 26, 2020

View reviewed changes

DeeDeeG merged commit 0d97024 into RefugeRestrooms:develop Oct 26, 2020

DeeDeeG removed the Ready for Review label Oct 26, 2020

DeeDeeG mentioned this pull request Oct 31, 2020

Release 1.20.0 #655

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

GPrimola commented Oct 18, 2020

DeeDeeG commented Oct 20, 2020

DeeDeeG Oct 20, 2020 •

edited

Loading

GPrimola Oct 20, 2020

DeeDeeG Oct 21, 2020

GPrimola Oct 21, 2020

DeeDeeG commented Oct 23, 2020 •

edited

Loading

DeeDeeG commented Oct 23, 2020

GPrimola commented Oct 24, 2020

DeeDeeG commented Oct 24, 2020

GPrimola commented Oct 24, 2020

GPrimola commented Oct 25, 2020

DeeDeeG commented Oct 26, 2020

DeeDeeG commented Oct 26, 2020 •

edited

Loading

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

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

Conversation

GPrimola commented Oct 18, 2020

Context

Summary of Changes

Checklist

Screenshots

Before

After

DeeDeeG commented Oct 20, 2020

DeeDeeG Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

GPrimola Oct 20, 2020

Choose a reason for hiding this comment

DeeDeeG Oct 21, 2020

Choose a reason for hiding this comment

GPrimola Oct 21, 2020

Choose a reason for hiding this comment

DeeDeeG commented Oct 23, 2020 • edited Loading

DeeDeeG commented Oct 23, 2020

GPrimola commented Oct 24, 2020

DeeDeeG commented Oct 24, 2020

GPrimola commented Oct 24, 2020

GPrimola commented Oct 25, 2020

DeeDeeG commented Oct 26, 2020

DeeDeeG commented Oct 26, 2020 • edited Loading

DeeDeeG Oct 20, 2020 •

edited

Loading

DeeDeeG commented Oct 23, 2020 •

edited

Loading

DeeDeeG commented Oct 26, 2020 •

edited

Loading