WMS popup functionality #173

JorgeMartinezG · 2021-04-23T20:45:20Z

This PR closes #134

On a map click event, given the case that a selected layer has featureInfoProps array, prism executes a getFeatureInfo request to get information. The result is filtered given the labels in the array. For this PR, is used for mozambique wind buffers layer.

Deployment found here: http://homeless-giraffe.surge.sh/

wadhwamatic

Can we allow for the configuration to give a label to the returned field names?
event_name = Event name
pub_date = Publication date

And can we return the date in a more readable format, so the popup would read:
Event name: CHALANE-20
Publication date: 2020-12-30 07:34 UTC

JorgeMartinezG · 2021-04-24T00:19:22Z

Sure. Included in 7c4018a and redeployed to same url: http://homeless-giraffe.surge.sh

wadhwamatic · 2021-04-24T00:25:31Z

Awesome. Great work Jorge. Looks good from my perspective. Over to @ericboucher for code review.

Ry-DS

Nice! Left some comments to help make the code more organized

Ry-DS · 2021-04-29T07:18:23Z

src/components/MapView/DateSelector/index.tsx

@@ -48,7 +48,11 @@ const Input = forwardRef(
    );
  },
 );
-function DateSelector({ availableDates = [], classes }: DateSelectorProps) {
+function DateSelector({


I would use React.forwardRef, but in the future if we need a ref to the selector we'll run into issues. Up to you 👍

I used a ref just to know current value from DateSelector. In both cases we would need to create the ref within the Mapview component and pass it to the DateSelector one.

Another option would be to use Localstorage, but some additional change will be needed within the addLayer reducer function

src/components/MapView/Layers/ImpactLayer/index.tsx

src/components/MapView/Layers/raster-utils.ts

Ry-DS · 2021-04-29T07:23:49Z

src/components/MapView/index.tsx

@@ -211,8 +278,43 @@ function MapView({ classes }: MapViewProps) {
        containerStyle={{
          height: '100%',
        }}
-        onClick={() => {
+        onClick={(map: Map, evt: any) => {


Any chance the library comes with a type for this? Would be nice to not use any. If it doesn't we could make an alias to any called MapEvent, or a bare bones type which has the fields we need.

I decided to use the any type, since many mapbox handlers use it as type across the project. It would be better to use any to keep consistency. If not, a change is needed for all mapbox event handlers with their respective type. But this should be addressed on another issue

src/components/MapView/index.tsx

src/config/types.ts

src/utils/server-utils.ts

Ry-DS · 2021-04-29T07:51:33Z

src/utils/server-utils.ts

+  const requestLayers = layers.filter(l => l.baseUrl === url);
+  const layerNames = requestLayers.map(l => l.serverLayerName).join(',');
+
+  const requestParams = {


If there's a type that lines up with this, we should use it:
const requestParams: <Type> ={...}

Included in 075838b. Needed to add a CamelCase to snake transformation function

Use lodash's snakeCase function https://lodash.com/docs/4.17.15#snakeCase

Ry-DS · 2021-04-29T07:52:48Z

src/utils/server-utils.ts

+
+  const wmsParams = { ...params, ...requestParams };
+
+  return fetch(formatUrl(`${url}/ows`, wmsParams))


To make this more readable the async await paradigm could be used, so all these intermediary steps can just be await <task>

Included in b1d8b95

src/components/MapView/index.tsx

JorgeMartinezG · 2021-04-30T21:25:35Z

@Ry-DS Feedback implemented on most of the comments. Please check

Ry-DS · 2021-05-05T04:02:08Z

@JorgeMartinezG
did you address these?
#173 (comment)
#173 (comment)

They were hidden by GitHub because there were too many review comments haha. Let me know what you think of those 👍

Ry-DS

Getting closer! Make sure to check out the comments I linked above, and to wrap up this PR I'd like if we spent some time to ensure our types are as strong as they can be.

Ry-DS · 2021-05-05T04:15:51Z

src/components/MapView/index.tsx

+          const params = getFeatureInfoParams(map, evt, dateFromRef);
+          makeFeatureInfoRequest(featureInfoLayers, params).then(
+            (result: any) => {
+              if (result === null) {


falsey check instead just in case the value is undefined?

Included in 30953dd

src/config/types.ts

Ry-DS · 2021-05-05T04:17:26Z

src/utils/server-utils.ts

+  url: string,
+  wmsParams: requestFeatureInfo,
+  layers: WMSLayerProps[],
+): Promise<{ [name: string]: string }> {


On line 318 the API result is typed well, so we should be able to provide a good type as the output for this function. This can be done by typing the reduce functions used in this file. What do you think? We should always try to make our types as strong as possible.

Ry-DS · 2021-05-05T04:17:36Z

src/utils/server-utils.ts

+  layers: WMSLayerProps[],
+): Promise<{ [name: string]: string }> {
+  // Transform to snake case.
+  const toSnakeWmsParams = Object.entries(wmsParams).reduce(


Replace with lodash's snakeCase function.
https://lodash.com/docs/4.17.15#snakeCase

included in 73987d1

Might be clearer if called wmsParamsInSnakeCase, what do you think?

Ry-DS · 2021-05-05T04:19:14Z

src/utils/server-utils.ts

+  layers: WMSLayerProps[],
+  url: string,
+  params: FeatureInfoType,
+): Promise<{ [name: string]: string }> {


Don't define the return type here and just let TS infer it. The plan (imo) is to strongly type the return of runFeatureInfoRequest and allow TS to infer the same type since you directly return the output of runFeatureInfoRequest

Ry-DS · 2021-05-05T04:21:51Z

src/utils/server-utils.ts

+export async function makeFeatureInfoRequest(
+  layers: WMSLayerProps[],
+  params: FeatureInfoType,
+): Promise<{ [name: string]: string } | null> {


Unless you're using an api without strong types, or an untyped library, we shouldn't make lose types like this. FOr MapEvent , it is fair to use any since the library doesn't give us a type, but from what I see we can make strong types using GeoJSON.FeatureCollection. If you can't see an easy way to create types for this let me know :)

Geojson standard has a strong type, but properties field is a key, value container. It lets flexibility for including a wide range of possible keys and values. For PRISM and WFS, we extract the values from keys included within the layers.json file. I mean, we could add a type constraint for the response. However it would work only for a single layer, since ADAM have different property schemas accross its layers

I'm not diving into the details here, but would like to add that flexibility on the types of data returned from the WFS feature request is important. We're solving for one dataset now (ADAM tropical storms), but more will come down the road, and we won't know how information is structured for each layer until we get into the integration steps. So being able to manage various types within the configuration of the layer is preferred.

Ry-DS · 2021-05-05T04:23:12Z

src/utils/server-utils.ts

+  const requests = urls.map(url => fetchFeatureInfo(layers, url, params));
+
+  return Promise.all(requests)
+    .then(r => r.reduce((obj, item) => ({ ...obj, ...item }), {}))


All these types will be stronger if we start using types for all the reduce calls in this file:

arr.reduce<{output: string}>((arg1, arg2)=>{},{}); //or arr.reduce((arg1, arg2)=>{},{} as {output: string});

…sm-frontend into feature/134-wms-feature-info

ericboucher · 2021-05-11T22:54:02Z

Looking great @JorgeMartinezG! Let us know when you had a chance to look at the remaining comments

…sm-frontend into feature/134-wms-feature-info

JorgeMartinezG · 2021-05-17T14:29:59Z

I'm not sure, but most of the feedback has been addressed already. What was missing is some indicator in the popup that additional information is loading. I decided to add a LinearProgress and is addressed here: e8d0e89

Also, a change in a variable name in here: 1d5057e

Please note that popup now is limited only within country boundaries, since two different events (map and layer) might be triggered at the same time, two popups might appear.
Also, what was mentioned previously related to WFS response, we cannot know previously the response type within properties field, since schemas per layer differ. For instance, wind buffers has a label, and nodes has not.

This branch has been deployed to this endpoint: http://homeless-giraffe.surge.sh/

WMS popup functionality

447082d

JorgeMartinezG requested review from wadhwamatic and ericboucher April 23, 2021 20:45

wadhwamatic reviewed Apr 23, 2021

View reviewed changes

Include data parsing for wms getfeatureinfo request

7c4018a

ericboucher requested a review from Ry-DS April 28, 2021 16:50

ericboucher added 2 commits April 28, 2021 12:11

Merge branch 'master' into feature/134-wms-feature-info

e618c0c

Merge branch 'master' into feature/134-wms-feature-info

fe105db

Ry-DS reviewed Apr 29, 2021

View reviewed changes

JorgeMartinezG added 9 commits April 29, 2021 08:45

Merge branch 'master' into feature/134-wms-feature-info

75e0a2d

Move comment to function

434bf4e

Refactor getExtent function

480a555

Remove undefined return within getActiveFeatureInfoLayers function

917c9da

use PascalCase for enum type

a385448

Modify featureInfo response format function

f4b9979

Include interface for wms feature info request params

075838b

fetch featureInfoRequest as async function

b1d8b95

Create utils file for MapView component

e9b782b

Merge branch 'master' into feature/134-wms-feature-info

77b11ce

ericboucher requested a review from Ry-DS May 5, 2021 02:46

Ry-DS reviewed May 5, 2021

View reviewed changes

JorgeMartinezG added 5 commits May 5, 2021 11:30

Change to PascalCase

f37d4fe

Merge branch 'feature/134-wms-feature-info' of github.com:WFP-VAM/pri…

4972eb3

…sm-frontend into feature/134-wms-feature-info

Missing save

ca25a0d

Validate undefined and null for map onclick event

30953dd

use SnakeCase function from lodash

73987d1

JorgeMartinezG and others added 2 commits May 5, 2021 11:41

Use moment lib to parse selected date from calendar

c55874f

Merge branch 'master' into feature/134-wms-feature-info

edf0c32

ericboucher and others added 5 commits May 16, 2021 23:24

Merge branch 'master' into feature/134-wms-feature-info

20f56b2

Merge branch 'master' into feature/134-wms-feature-info

d6c3797

Modify variable name

1d5057e

Include LinearProgress within tooltip component

e8d0e89

Merge branch 'feature/134-wms-feature-info' of github.com:WFP-VAM/pri…

25c2bd7

…sm-frontend into feature/134-wms-feature-info

ericboucher approved these changes May 17, 2021

View reviewed changes

ericboucher merged commit 55f51dc into master May 17, 2021

ericboucher deleted the feature/134-wms-feature-info branch May 17, 2021 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WMS popup functionality #173

WMS popup functionality #173

JorgeMartinezG commented Apr 23, 2021

wadhwamatic left a comment

JorgeMartinezG commented Apr 24, 2021

wadhwamatic commented Apr 24, 2021

Ry-DS left a comment

Ry-DS Apr 29, 2021

JorgeMartinezG Apr 29, 2021

Ry-DS Apr 29, 2021

JorgeMartinezG Apr 29, 2021

Ry-DS Apr 29, 2021

JorgeMartinezG Apr 30, 2021

Ry-DS May 5, 2021

Ry-DS Apr 29, 2021

JorgeMartinezG Apr 30, 2021

JorgeMartinezG commented Apr 30, 2021

Ry-DS commented May 5, 2021

Ry-DS left a comment

Ry-DS May 5, 2021

JorgeMartinezG May 5, 2021

Ry-DS May 5, 2021 •

edited

Ry-DS May 5, 2021

JorgeMartinezG May 5, 2021

ericboucher May 5, 2021

Ry-DS May 5, 2021 •

edited

Ry-DS May 5, 2021 •

edited

JorgeMartinezG May 5, 2021 •

edited

wadhwamatic May 6, 2021

Ry-DS May 5, 2021

ericboucher commented May 11, 2021

JorgeMartinezG commented May 17, 2021 •

edited


		const wmsParams = { ...params, ...requestParams };

		return fetch(formatUrl(`${url}/ows`, wmsParams))

WMS popup functionality #173

WMS popup functionality #173

Conversation

JorgeMartinezG commented Apr 23, 2021

wadhwamatic left a comment

Choose a reason for hiding this comment

JorgeMartinezG commented Apr 24, 2021

wadhwamatic commented Apr 24, 2021

Ry-DS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorgeMartinezG commented Apr 30, 2021

Ry-DS commented May 5, 2021

Ry-DS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ry-DS May 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ry-DS May 5, 2021 • edited

Choose a reason for hiding this comment

Ry-DS May 5, 2021 • edited

Choose a reason for hiding this comment

JorgeMartinezG May 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericboucher commented May 11, 2021

JorgeMartinezG commented May 17, 2021 • edited

Ry-DS May 5, 2021 •

edited

Ry-DS May 5, 2021 •

edited

Ry-DS May 5, 2021 •

edited

JorgeMartinezG May 5, 2021 •

edited

JorgeMartinezG commented May 17, 2021 •

edited