Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Manual bbox entry option to search page #389

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

stephenkilbourn
Copy link

As requested in PR #302 , This is breaking out a new feature into its own PR.

AreaSelect BBoxEntry
defaultAreaSelect newBBoxEntry

This PR is intended to add a new BBEntry component that allows the entry of [x_min, y_min, x_max, y_max] instead of using a bounding box selection on the map. This new component should help accessibility and help make it clear the coordinates being searched by a user.

The default behavior on the search page is still to show the areaSelect Component if a user checks the Filter by spatial extent option. However, now there is a radio button option to toggle to "Enter coordinates". This option hides the area select and allows a user to manually enter the coordinates.

The input boxes are limited to numbers ±180 or ±90 via the bootstrap input component.

I did add two English terms that would need translations. If there is a way for me to request that via crowdin, then I can do that:

    "defineBbox": {
      "map": "Draw bounding box on map",
      "text": "Enter coordinates"
    },

<b-form-group>
<b-form-row>
<b-col>
<b-form-group label="x_min" label-for="x_min">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this is usually done in forms, but x_min seems a little too technical as a user readable label.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked around and didn't find a clear answer. I could do something like Lat max/min and Lon max/min. Would that be clearer to most users?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe follow the specification terms? I guess there are names hidden in the API spec documents.

Copy link
Author

@stephenkilbourn stephenkilbourn Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the geojson spec, the bounding box uses the terms "southwest" and "northeast" and avoids min/max to avoid confusion when crossing the antimeridian. I've updated the labels to Southwest latitude/longitude and Northeast latitude/longitude to match that.
bbox

@m-mohr
Copy link
Collaborator

m-mohr commented Nov 10, 2023

Thanks for the PR. Is the bbox synced between map and form inputs?

@stephenkilbourn
Copy link
Author

The coordinates drawn with the area select on the map sync to the form inputs. I.e. a user can draw a box, and then switch to the form input and continue to manually edit them.

Unless I missed something, the areaSelect doesn't accept a starting bounding box to visually set the handles. Right now, switching from the form inputs back to the bounding box resets the query.bbox value back to the default, so a user would need to re-draw.

The different behaviors does feel strange. I could make it reset any time a user changes tabs to be consistent. Ideal scenario might be updating the the areaSelect to display the bounds and show both manual and visual map entries at the same time with state synced instead of on separate tabs.

@m-mohr
Copy link
Collaborator

m-mohr commented Nov 10, 2023

I guess the area select tool needs some work so that it can be synced. It's a very simple thing that I found on the internet, but I guess others have solved that better somewhere...

@stephenkilbourn
Copy link
Author

with the latest update, submitting the search will submit the bbox on whichever tab was last edited. i.e. if a user edits with the manual entry, then switches to the map tab without drawing there, the bbox used in the search is still the one from the manual text entry. The area select still does not update what is displayed, though.

@stephenkilbourn
Copy link
Author

ok, I added a commit that gave the area select an setInitialBounds method. That will set the area box height and width to the same value manually entered on the manual entry tab

<b-container>
<b-form-row>
<b-col>
<b-form-group label="Southwest Latitude" label-for="sw_latitude">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The labels needs to be added to the locales.

@@ -36,6 +38,20 @@ L.AreaSelect = L.Class.extend({
return this;
},

setInitialBounds: function ( bounds) {
Copy link
Collaborator

@m-mohr m-mohr Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just found my old code from another tool again:

    setBounds: function(bounds) {
        if (Array.isArray(bounds) && bounds.length >= 4) {
          bounds = L.LatLngBounds([bounds[0], bounds[1]], [bounds[2], bounds[3]]);
        }
        this.map.fitBounds(bounds);

        var bottomLeft = this.map.latLngToContainerPoint(bounds.getSouthWest());
        var topRight = this.map.latLngToContainerPoint(bounds.getNorthEast());

        this.setDimensions({
            width: Math.abs(bottomLeft.x - topRight.x),
            height: Math.abs(bottomLeft.y - topRight.y)
        });
    },

Here's the whole code for reference:
https://github.com/Open-EO/openeo-web-editor/blob/5ec303fc023b5a745254ea34f6218337fed89ae7/src/components/leaflet-areaselect/leaflet-areaselect.js

I think this might be a bit more universal and supports the Leaflet Bounds for consistency with getBounds. Would you mind updating it?

src/components/Map.vue Outdated Show resolved Hide resolved
@m-mohr
Copy link
Collaborator

m-mohr commented Jan 12, 2024

Thanks for the updates. Left a couple of comments, but otherwise looks good.

@stephenkilbourn
Copy link
Author

thanks for reviewing @m-mohr - comments addressed. Things are working when I test locally against https://earth-search.aws.element84.com/v1/. Let me know if there are other issues.

@m-mohr m-mohr self-requested a review January 13, 2024 13:45
@m-mohr
Copy link
Collaborator

m-mohr commented Jan 15, 2024

Thank you for the updates. I just did another review and executed this locally. There's a bug when changing the bbox in manually and then switching to the map:
bbox
This needs further investigation.

@stephenkilbourn
Copy link
Author

I was not moving the map to center on the manually entered bounding box when a user went back to the map view.

I added to commits with a few changes to address that bug and other potential ones:

  • With a little more testing around that, I realized a user could put also enter values for lat & lng in the Southwest corner that were larger than the NE corner values. I added some better validation there.

  • There was also some shift in the exact values of bounding box on the map vs what was entered in the manual entry tab due to rounding. The map's resolution doesn't allow very precise drawings, but the leaflet function returns a float. A 73.xxxxx1 in the manual entry would shift some when switching to the map tab and then back. I added a check to not get the new bounds on adding the area select if the bounds are already in state. I also updated to round the lat & lng on the bounding box to an integer if the Map view is used to select a bounding box.

@m-mohr
Copy link
Collaborator

m-mohr commented Mar 15, 2024

Hey there,
sorry for the slow follow-up and thanks for the updates. I tested this again and it works better now, but the rounding seems to be flawed when smaller areas (e.g. a small pond or village) is selected. Then the rounding may lead to the following:

grafik

Also, for these smaller extents I believe the map should be zoomed to roughly fit to the extent and only then the bbox should be drawn. Otherwise it looks like this:

grafik

The main paint point here is the bad bbox chooser. Ideally we'd get a better one...

@m-mohr m-mohr added this to the 3.5.0 milestone Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants