Store initial map interaction settings

[alexgleith]

Hey, this is a first draft of a change, which is intended to enable storage and consistent setting of map interaction settings when enabling Draw.

There's a couple of concepts in here:

1. The initial state is set and restored in setup.js using the Store
2. The initial state can be queried in the toggling of interactions so that settings are maintained
3. For now, it's been implemented in the double_click_zoom.js file and tested in the debug map and works well.

This PR is really a first pass, where I'm hoping to get feedback on the concept. After feedback I'll finish implementation for the other interactions.

I have an issue between the develop mode interactive site and the tests, in that the store and map are different, so either one or the other fails. This may be (probably is) my fault, so feedback in how to handle that would be appreciated.

This is affecting a app I'm building, so I'm keen to work on a fix now =).

[alexgleith]

Just a note here that I'm aware that this test 👇 fails, but the code there works on the "debug" map.

[mcwhittemore]

@alexgleith Thanks for this PR. Sorry for the slow reply.

This is a great idea. It would be good to have a test that confirms we're tracking all the right values in mapbox-gl. I don't think these are going to change, but having the test suite fail if they do will be really helpful.

Per the failing tests, our tests use mapbox-gl-js-mock and it looks like some of these options aren't mocked out there. We should probably rethink how we do this testing, but that can be a different PR.

[alexgleith]

Hey @mcwhittemore, thanks for the feedback.

I get what your saying about the options in the mock thingy. But I'm not really sure how to handle it.

If I were to get this PR working with all the interaction switchers (like double_click_zoom.js) would it be likely to be merged? If so, I'll do that work next week. If my method is not right, let me know, and I'll refactor it then and resubmit.

[mcwhittemore]

@alexgleith - As long as the tests are passing and the functionality is complete this will merge pretty quickly after the final PR. That said, I don't have much time right now to help clear up the failing tests.

[alexgleith]

@mcwhittemore, ok, so I should address the failing tests as part of the PR then, even though they are failing for the wrong reasons?

[mcwhittemore]

@alexgleith - yea. Tests aren't failing on master, so its the additions in this PR that are breaking the test suite. That needs to be resolved before being brought into master.

[alexgleith]

Hey @mcwhittemore, I think this is ready to go.

[mcwhittemore]

Thanks for this @alexgleith. Can you add one automated test to confirm that this feature works. I want to know that we won't regress on this.

[alexgleith]

Hey @mcwhittemore, I think this will be really hard to do a test for.

For example, to test it properly, we should really do the following:

1. Create a map object with doubleClickZoom disabled
2. Add the Draw control to it
3. Use one of the Draw functions that enables/disables doubleClickZoom
4. Remove the Draw control
5. Check the map object's doubleClickZoom state.

This sounds reasonable, but the MapBoxGL Mock object doesn't do half of these things... so to test, it means either creating my own mock process, or improving the upstream one.

I'll have a look at just checking the two functions storeMapConfig and restoreMapConfig now, maybe that will suffice.

[alexgleith]

Hey @mcwhittemore, I've implemented one set of tests, and that was very useful because it did highlight a bug already (doh!). I'm still unsure on how to mock up removing the draw control, so that we can test whether that part works. I've tested it in the debug app, though, and we're solid 👍.

[mcwhittemore]

Thanks @alexgleith! This has been merged and published to NPM in v1.0.4

[locomotif]

Hi! I noticed this on my test output.

[alexgleith]

Oh, crap. This is already merged. Should I create another PR to fix?

[mcwhittemore]

That would be great. Thanks! Can you look at adding no console.log to the eslint config as well?

[alexgleith]

This has been done, see: #716