Clean up map view info window #418

stardust66 · 2018-01-11T03:32:55Z

Context

Fixes Cleanup the map view's marker box #352.

Summary of Changes

Replace the NumberedCircle class by using Google's built-in custom marker and info window system. For details please see my commit message.
Make the filter icons horizontal, as suggested in the issue.

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

Using the built-in marker eliminates the need for the NumberedCircle class and significantly simplifies the code, since now the Google API takes care of positioning the marker, the label, and the info window. This also solves the overflow and lack of padding problems that the old info window had.

DeeDeeG · 2018-01-11T17:45:40Z

Some tests failed, so I'll go look into that, but the screenshot sure looks nice!

DeeDeeG · 2018-01-11T18:32:15Z

For some reason this breaks a few cucumber tests.

There is a supporting ruby function, mock_location, for 4 of the tests that actually just executes a javascript function on the page.

see: features/support/locations.rb which contains the underlying functions that are error-ing out.

That set of functions errors out for 4 of the tests now.

These are:
cucumber features/nearby.feature:4 # Scenario: Show nearby restrooms when guessing
cucumber features/nearby.feature:9 # Scenario: Show absence of nearby restrooms
cucumber features/search.feature:10 # Scenario: Location search from splash page
cucumber features/search.feature:20 # Scenario: Map display

The error messages look pretty much like this all 4 times:

    When I am in Winnipeg and I guess my location on the submission page # features/step_definitions/submission_steps.rb:29
      One or more errors were raised in the Javascript code on the page. If you don't care about these errors, you can ignore them by setting js_errors: false in your Poltergeist configuration (see documentation for details).
      ReferenceError: Can't find variable: Refuge
      ReferenceError: Can't find variable: Refuge
          at undefined:4
          at :21
          at :21 (Capybara::Poltergeist::JavascriptError)
      ./features/support/locations.rb:12:in `mock_location'
      ./features/step_definitions/submission_steps.rb:33:in `/^I am in (.*) and I guess my location on the submission page$/'
      features/nearby.feature:6:in `When I am in Winnipeg and I guess my location on the submission page'
    Then I should see an existing restroom nearby                        # features/step_definitions/restroom_steps.rb:9

(The innermost error is "Can't find variable: Refuge".)

There are also two tests where searching for an HTML element by its #id and/or .class fails:

Here
cucumber features/search.feature:4 # Scenario: Text search from splash page

    When I am on the splash page         # features/step_definitions/navigation_steps.rb:1
    And I search for "Winnipeg"          # features/step_definitions/search_steps.rb:1
    Then I should see a restroom         # features/step_definitions/restroom_steps.rb:5
      expected to find visible css "#list .listItem" but there were no matches (RSpec::Expectations::ExpectationNotMetError)
      ./features/step_definitions/restroom_steps.rb:6:in `/^I should( not)? see a restroom$/'
      features/search.feature:8:in `Then I should see a restroom'

and Here
cucumber features/preview.feature:4 # Scenario: Preview a restroom before submitting

  Scenario: Preview a restroom before submitting    # features/preview.feature:4
    Given I have filled out the address information # features/step_definitions/preview_steps.rb:1
    When I click the preview button                 # features/step_definitions/preview_steps.rb:11
    Then I should see the map preview               # features/step_definitions/preview_steps.rb:15
      expected to find visible css "div#mapArea" but there were no matches. Also found "", which matched the selector but not all filters. (RSpec::Expectations::ExpectationNotMetError)
      ./features/step_definitions/preview_steps.rb:16:in `/^I should see the map preview$/'
      features/preview.feature:7:in `Then I should see the map preview'

stardust66 · 2018-01-12T17:03:18Z

I don't know what's making the tests not pass. The website seems to exhibit normal behavior when I use it, i.e. it searches and displays bathrooms correctly. I'll try and debug the tests but that might take a while since I'll be busy over the next few days.

DeeDeeG · 2018-01-13T05:38:08Z

I agree that the code looks normal to me, and it definitely works. These errors are also really strange to me.

In order to hopefully save some time on manual debugging, or at least narrow the bug hunt a bit, it might be worth submitting each commit to a PR one at a time. That would let Travis CI do a full run on each commit.

I have in mind a way to do this from the command line, but perhaps you already know exactly how, so feel free to ignore; Regardless, I'm not going to hint at a way without explaining it, so here goes:

If you preserve these commits (for example, just make an identical branch with git branch -b [any-name-here])...

you can switch back to (git checkout) the map-bathroom-detail branch, and then git reset --hard HEAD~3 (reset this branch to the commit at 3 commits back from the HEAD.)

and then merge your commits back, one at a time, i.e:

git merge 3f7f316 (which would automatically grab the first of the three commits for this PR, from your copy branch mentioned above)

and then git push https://github.com/stardust66/refugerestrooms

This is the standard approach and having mixed tabs and spaces isn't good.

Taking the outer html of the container element to the container itself. This is useful because the container has the markerPopup class.

stardust66 · 2018-01-14T20:18:15Z

It seems like the first commit already broke the tests.

DeeDeeG · 2018-01-15T03:56:37Z

From here on the first commit's tests:

Failing Scenarios:
cucumber features/nearby.feature:4 # Scenario: Show nearby restrooms when guessing
cucumber features/nearby.feature:9 # Scenario: Show absence of nearby restrooms
cucumber features/preview.feature:4 # Scenario: Preview a restroom before submitting
cucumber features/search.feature:4 # Scenario: Text search from splash page
cucumber features/search.feature:10 # Scenario: Location search from splash page
cucumber features/search.feature:20 # Scenario: Map display

The tests failing on the first commit are the same as the ones that failed previously for the PR as a whole. All that from changing the whitespace in the maps javascript file? Kinda weird.

stardust66 · 2018-01-15T03:59:34Z

Actually, the first commit is right below the top post. It's titled "Use google maps built-in custom marker". In there, I deleted the NumberedCircle class, and replaced it with a Google Maps custom marker with label.

DeeDeeG · 2018-01-15T04:22:45Z

Okay, yep. Anyway, failing tests are the same after commit 1, 2, and 3, i.e. nothing additional breaks later than commit 1, so I guess the troubleshooting can be limited to commit 1. Hopefully that narrows it down a bit. (I'm not that great with JavaScript, but I'll look it over anyway.)

stardust66 · 2018-01-15T04:24:18Z

Thank you @DeeDeeG! I'm gonna be busy over the next couple of days so I probably won't be able to work on this that much.

DeeDeeG · 2018-01-16T05:13:08Z

So we're assuming there is some obscure thing wrong with maps.js, since that's the only change in commit one.

According to this online JavaScript validator, [link], there are some undefined functions (among other, possibly more-minor errors):

Line	Column	Error
4	52	'currentLocationImage' is not defined.
54	10	'circleMarkerImage' is not defined.
143	20	'locator' is not defined.
182	64	'showMarkerImage' is not defined.

Maybe that has something to do with it?

stardust66 · 2018-01-28T05:51:49Z

@DeeDeeG Thanks for taking a look. Those are all variables defined outside of maps.js that are automatically included in the <head> of every page by rails. The image ones are in global_vars.js.erb.

During testing, working from the original commit, I've deleted the entire NumberedCircle class, which would cause the bathrooms to not show up on the map at all, and the tests still pass. Taking a further look, most of the steps for testing the map feature have been previously commented out, in map_steps.rb. The tests that are failing for this PR don't pertain to the map feature.

This is what was breaking the tests. The headless browser doesn't support arrow functions, which most modern browsers support now. This was breaking all the tests.

stardust66 · 2018-01-28T06:21:45Z

@DeeDeeG Fixed it! In the end the problem was me using an arrow function () => {}, which the headless browser that ran the tests didn't support. Although every up to date browser other than internet explorer supports it, I probably shouldn't have used it because we want to support the internet explorer users. However, I think we should make the tests a little easier to debug.

DeeDeeG · 2018-01-28T18:38:04Z

Nice! So this looks merge-ready to me. (Ideally reviewed by someone who gets JavaScript better than I do, but this seems pretty simple.)

Any thoughts on how to make the tests better?

Maybe some broader (less hyper-narrow, less feature-oriented) error steps, for example add a more generic "page should not show a big huge error message"?

(Not that that would help with this PR, I think.)

Maybe there is a way to make PhantomJS, the headless browser we use, surface an error message of its own?

stardust66 · 2018-01-28T19:00:33Z

I just made #432 and #433 regarding test debugging.

DeeDeeG · 2018-01-28T21:04:57Z

@mi-wood, would you like to review this, now that the Travis CI build is passing?

I think it would be better to have a reviewer who knows more about JavaScript, but I am comfortable with merging it, since it works in testing.

mi-wood

Minor comment, but I this seems to get rid of a lot of code, which is nice :)

My one question is if this makes #298 harder? Do the helper map classes included in their licensing, or is it separate from the data? And if we decide to move off of google maps, will these need to be changed back to our custom objects?

mi-wood · 2018-01-28T21:28:07Z

app/assets/javascripts/maps.js

+  // Create Icon
+  var circleIcon = {
+    url: circleMarkerImage,
+    size: new google.maps.Size(37, 37),


Where are these values coming from? Can we move them out into constants DEFAULT_WIDTH or something?

I measured the size of the image. It's in a constant now.

stardust66 · 2018-01-29T23:09:33Z

As per your question, when we get to #298, we'll probably need to use another library to do this, so it'll be a bit of work, but the old solution depended on the Google Maps API as well, just not as much. Also, it'll be a better idea to use a library to avoid reinventing the wheel, so I think moving on we just won't use custom objects.

From my understanding, OpenStreetMap is mostly just the data, and to deploy it we need some other library. Leaflet, suggested by https://wiki.openstreetmap.org/wiki/Deploying_your_own_Slippy_Map, seems like a good choice and it has a powerful API for markers and popups.

mi-wood · 2018-01-30T00:21:22Z

I'm good with this 👍

…etail Clean up map view info window

stardust66 force-pushed the map-bathroom-detail branch from 8266608 to 0ccc52a Compare January 11, 2018 04:00

stardust66 force-pushed the map-bathroom-detail branch from 0ccc52a to 3f7f316 Compare January 13, 2018 12:35

stardust66 added 2 commits January 14, 2018 09:52

Replace all tabs with spaces in maps.js

469b29b

This is the standard approach and having mixed tabs and spaces isn't good.

Make filters icons horizontal

a89ef11

Taking the outer html of the container element to the container itself. This is useful because the container has the markerPopup class.

Change arrow function to old style function

10d5f83

This is what was breaking the tests. The headless browser doesn't support arrow functions, which most modern browsers support now. This was breaking all the tests.

mi-wood reviewed Jan 28, 2018

View reviewed changes

Store map marker width in constant

4c1b7de

Merge branch 'develop' into map-bathroom-detail

32c5de2

mi-wood approved these changes Jan 30, 2018

View reviewed changes

mi-wood merged commit 3505b12 into RefugeRestrooms:develop Jan 30, 2018

stardust66 deleted the map-bathroom-detail branch January 30, 2018 02:36

DeeDeeG pushed a commit to DeeDeeG/refugerestrooms that referenced this pull request Oct 15, 2018

Merge pull request RefugeRestrooms#418 from stardust66/map-bathroom-d…

8f98cd4

…etail Clean up map view info window

DeeDeeG pushed a commit to DeeDeeG/refugerestrooms that referenced this pull request Nov 3, 2018

Merge pull request RefugeRestrooms#418 from stardust66/map-bathroom-d…

932761f

…etail Clean up map view info window

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up map view info window #418

Clean up map view info window #418

stardust66 commented Jan 11, 2018

DeeDeeG commented Jan 11, 2018

DeeDeeG commented Jan 11, 2018 •

edited

stardust66 commented Jan 12, 2018

DeeDeeG commented Jan 13, 2018 •

edited

stardust66 commented Jan 14, 2018

DeeDeeG commented Jan 15, 2018

stardust66 commented Jan 15, 2018

DeeDeeG commented Jan 15, 2018

stardust66 commented Jan 15, 2018

DeeDeeG commented Jan 16, 2018 •

edited

stardust66 commented Jan 28, 2018

stardust66 commented Jan 28, 2018

DeeDeeG commented Jan 28, 2018 •

edited

stardust66 commented Jan 28, 2018

DeeDeeG commented Jan 28, 2018

mi-wood left a comment

mi-wood Jan 28, 2018

stardust66 Jan 29, 2018

stardust66 commented Jan 29, 2018 •

edited

mi-wood commented Jan 30, 2018

Clean up map view info window #418

Clean up map view info window #418

Conversation

stardust66 commented Jan 11, 2018

Context

Summary of Changes

Checklist

Screenshots

Before

After

DeeDeeG commented Jan 11, 2018

DeeDeeG commented Jan 11, 2018 • edited

stardust66 commented Jan 12, 2018

DeeDeeG commented Jan 13, 2018 • edited

stardust66 commented Jan 14, 2018

DeeDeeG commented Jan 15, 2018

stardust66 commented Jan 15, 2018

DeeDeeG commented Jan 15, 2018

stardust66 commented Jan 15, 2018

DeeDeeG commented Jan 16, 2018 • edited

stardust66 commented Jan 28, 2018

stardust66 commented Jan 28, 2018

DeeDeeG commented Jan 28, 2018 • edited

stardust66 commented Jan 28, 2018

DeeDeeG commented Jan 28, 2018

mi-wood left a comment

Choose a reason for hiding this comment

mi-wood Jan 28, 2018

Choose a reason for hiding this comment

stardust66 Jan 29, 2018

Choose a reason for hiding this comment

stardust66 commented Jan 29, 2018 • edited

mi-wood commented Jan 30, 2018

DeeDeeG commented Jan 11, 2018 •

edited

DeeDeeG commented Jan 13, 2018 •

edited

DeeDeeG commented Jan 16, 2018 •

edited

DeeDeeG commented Jan 28, 2018 •

edited

stardust66 commented Jan 29, 2018 •

edited