Feature #2246- view finder feature to fly to user-specified latitude, longitude pair

[ianguerin]

This feature allows the user to input two floating point numbers, with helpful error messages when they enter malformed queries, or values that are out of bounds (-90:90, -180:180), or non-numerics.

The input becomes focused when the "view finder" layer section is opened.

This PR also introduces two extremely slimmed down "libraries" that use implement some best practices for unit tests (hermetic state, function spies) without adding additional third party dependencies.

fixes #2246

[ianguerin]

Here's a demo video of the feature: https://drive.google.com/file/d/1Sukzcv3BjcxH3Pi2fNHLd1g2Oii7LNu7/view?usp=drive_link

@robyngit, there's a lot of code here and I expect some discussion about it! Please let me know where I strayed too far from the current codebase style

[robyngit]

From a quick scan of your changes @ianguerin, it looks fantastic! I'll share more feedback after I've had a chance to go through this more thoroughly.

@iannesbitt, if you have the time, could you please test out these changes in the DRP theme and make sure it meets that group's requirements?

[yvonnesjy]

I'm trying to figure out why you created this.templateVars.inputValue, instead of just using this.$el.find(.${classNames.input}). It seems like this block might be why... but I still don't fully understand. Can you explain?

[ianguerin]

The main reason that I need to track the input value is so that the UI remains consistent across re-renders. So in the template I also reference this value, otherwise the input would be cleared each time I manually re-render.
I think your questions makes it clear that there should be a comment to that effect where the templateVars object is initialized

[iannesbitt]

Hi @ianguerin—thank you for diving in and creating a working feature so quickly! The Cesium visualization is in desperate need of this feature, and the clients we work with will be very pleased to be able to use it.

This feature is overall very smoothly integrated. I have not been able to break it or cause anything ugly to happen, which is awesome. I very much appreciate the heads up to the user when they enter coordinates outside of the [-90,90], [-180,180] ranges. I do however have a couple of items to address—one that's pressing and two that would be nice-to-have.

Zoom level

It seems to me that the zoom level is set too high when entering a point. My guess is that when a user enters a point, they'd like to see a much lower (tighter) zoom on that point on the Cesium globe. The research team that requested this feature, for example, will want to zoom directly to their study sites. This zoom level feels like it will still require them to do some work to find their site.

One possibility would be to zoom tighter based on the number of decimals supplied by the user, so for example entering 42,-71 might give you a zoom level that includes most of the state of Massachusetts, whereas 42.7, -73.1 might zoom to the town of North Adams. A calculation that makes this possible in a 2-D map wouldn't be too hard I don't think—we could just zoom to a bounding box that includes ± the last decimal values in the query to get rough extents to zoom to. This might be somewhat harder in an application like Cesium, however, and may need its own feature request depending on whether others think this is a good idea.

To summarize: zoom level should be much lower at least, and at most may be able to be calculated based on the decimal precision.

Place-based search

This is more of a "reach" feature that we could consider adding in the future. However, there are several geocoder (place name/location) databases out there that we could piggyback to be able to allow place name searches (see OSMNames). For example, a search for New York, NY would return a coordinate pair or bounding box and then we could have Cesium center the map on roughly that location (40.7, -74.0).

To summarize: we should include a way to search by place names (in the future).

Conclusion

I will approve the PR when the zoom level is tightened, but we should also consider the following additions to accompany this feature:

• Zoom level based on decimal precision
• Search based on place name database

Thank you once again for submitting this PR, this is very useful already and represents a big step forward for MetacatUI-Cesium UX possibilities. I'm incredibly excited to use it myself and to introduce our clients to it!

[ianguerin]

Thanks for your review @iannesbitt, I've updated the zoom to 10km instead of 321km, hopefully that is a more useful starting point.
While I do think searching for places is a lot more useful, I think that belongs as a separate feature and development process (one that I'm actively researching now) because of the work it will take to select a geocoding service, and also build out the UI for displaying a potential multitude of results corresponding to a place name.

@robyngit, it came up today whether or not it would be a good idea to release this feature without support for place name search. I'm curious to hear your thoughts, but also curious if there is a way we can put this feature behind a feature flag for now, or if we even have support for enabling/disabling features flags.

[robyngit]

@yvonnesjy @iannesbitt thank you for adding your reviews, very helpful!! 🎉

@ianguerin thank you for being thorough and adding tests! I think they were passing initially but it looks like something changed? npm test gives 4 failures now:

[1]: ViewFinderView Test Suite
     > bad search queries
     > AssertionError: expected '' to contain 'Try entering a search query with two …'
    at Context.<anonymous> (js/specs/unit/views/maps/ViewFinderView.spec.js?v=undefined:110:50)
[2]: ViewFinderView Test Suite
     > bad search queries
     > AssertionError: expected '' to contain 'Try entering a search query with two …'
    at Context.<anonymous> (js/specs/unit/views/maps/ViewFinderView.spec.js?v=undefined:128:50)
[3]: ViewFinderView Test Suite
     > bad search queries
     > AssertionError: expected '' to contain 'Latitude values outside of the'
    at Context.<anonymous> (js/specs/unit/views/maps/ViewFinderView.spec.js?v=undefined:146:50)
[4]: ViewFinderView Test Suite
     > bad search queries
     > AssertionError: expected '' to contain 'Longitude values outside of the'
    at Context.<anonymous> (js/specs/unit/views/maps/ViewFinderView.spec.js?v=undefined:164:50)
::error::One or more MetacatUI tests failed. Test failure details can be viewed by running "npm view-tests". %0AFAILS:  4%0APASSES: 315%0AFailed Tests: %0A-------------%0A%0A[1]: ViewFinderView Test Suite%0A     > bad search queries%0A     > AssertionError: expected '' to contain 'Try entering a search query with two …'%0A    at Context.<anonymous> (js/specs/unit/views/maps/ViewFinderView.spec.js?v=undefined:110:50)%0A[2]: ViewFinderView Test Suite%0A     > bad search queries%0A     > AssertionError: expected '' to contain 'Try entering a search query with two …'%0A    at Context.<anonymous> (js/specs/unit/views/maps/ViewFinderView.spec.js?v=undefined:128:50)%0A[3]: ViewFinderView Test Suite%0A     > bad search queries%0A     > AssertionError: expected '' to contain 'Latitude values outside of the'%0A    at Context.<anonymous> (js/specs/unit/views/maps/ViewFinderView.spec.js?v=undefined:146:50)%0A[4]: ViewFinderView Test Suite%0A     > bad search queries%0A     > AssertionError: expected '' to contain 'Longitude values outside of the'%0A    at Context.<anonymous> (js/specs/unit/views/maps/ViewFinderView.spec.js?v=undefined:164:50)

Do you know what changed?

[ianguerin]

Thanks for taking a look @robyngit, I have the tests passing locally and have just pushed those changes.
I've also renamed things from "view finder" to "viewfinder", as well as adding a a map config value to decide whether or not to show this new UI (hidden by default). I've refactored the code in ToolbarView to make that logic a bit more reusable as I noticed all of the buttons in that toolbar had the same kind of logic.

I'm going to look into adding a pre-commit hook that checks to see if unit tests are all passing before committing changes. This should also be helpful in the future if we decide to add linting!

[robyngit]

I was so happy to see how you managed to write these tests for views!! I'll be making use of this strategy moving forward. The comments and suggestions I added are all super minor and I did not manage to break the view finder no matter what I entered in the search box. I appreciate that you worked it into the Toolbar following the existing pattern. Very nice work!

The only other comment to add is about docs in Toolbar View on this line. * @returns {HTMLElement} Returns the content container with the rendered view just needs to be updated to account for the change in what's returned from the method:

         * @returns {Object} Returns an object with the content container and the view
         * that was rendered in that container.
         * @property {HTMLElement} contentContainer The container for the toolbar section's
         * content
         * @property {Backbone.View} sectionContent The view that was rendered in the
         * content container

(GitHub wouldn't allow me to comment directly since this isn't one you made changes to).

[ianguerin]

All tests pass locally so I'm not sure what to make of: https://github.com/NCEAS/metacatui/actions/runs/7906483230/job/21581405581?pr=2258

[robyngit]

@ianguerin not sure what that fail was all about, but I reran the tests with success 🎉

[ianguerin]

@ianguerin not sure what that fail was all about, but I reran the tests with success 🎉

Super, anything needed before merging?

[robyngit]

Ok I made an issue for you to address the out-of-range coordinates behaviour here: #2270, which can happen in a later release. Otherwise it looks like all concerns were addressed, nice work! :) Looking forward to seeing this in action.