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

Added table of content to open layer details in map view. #1661

Merged
merged 17 commits into from
Dec 4, 2024

Conversation

ahmdthr
Copy link

@ahmdthr ahmdthr commented Jan 25, 2024

Resolves #1660
Resolves #1616

A new button to view layer details is added on the map view.

@ridoo
Copy link

ridoo commented Jan 25, 2024

@ahmdthr the PR still is marked as draft .. if this is intended, though, please ignore my comment

@ahmdthr ahmdthr marked this pull request as ready for review January 30, 2024 05:27
@ridoo
Copy link

ridoo commented Mar 7, 2024

@ahmdthr this one would have to be aligned to the latest changes of linked resources

@ridoo
Copy link

ridoo commented Mar 19, 2024

Thanks @ahmdthr

@giohappy the PR is now ready for review on your side

@ridoo ridoo added the enhancement New feature or request label Mar 19, 2024
@giohappy giohappy requested a review from dsuren1 May 6, 2024 12:36
@giohappy
Copy link

giohappy commented May 6, 2024

@ridoo @ahmdthr we're taking a look right now

@dsuren1
Copy link

dsuren1 commented May 7, 2024

@ahmdthr Kindly resolve the conflicts and align the code changes with the latest changes in master. Thanks!

@dsuren1 dsuren1 requested a review from giohappy May 17, 2024 10:28
@gannebamm
Copy link

@giohappy Sorry @ahmdthr was busy with other tasks and we need some additional days to sort out our ressources. Are we to late for lending a helping hand? @dsuren1 already did some changes and requested a review.

@giohappy
Copy link

I've tested the PR locally. The idea is good and it's really useful, but I have a few concerns:

  • every time a layer is selected in the TOC a new API request is sent to obtain the required information to enable (or not) the info button for the layer
  • the information that has been retrieved is not saved, so the same request is sent again every time a layer is selected

I think this could be improved if the following is implemented:

  • The response can be stored, somehow, inside the state of the component. This avoids making the same request for the same layer multiple times
  • But, the real point is: are these calls really needed? We already query both the map and the datasets endpoint when the map is loaded. Much information about datasets is retrieved at that time. Isn't this enough? If more attributes are required we can add them to the response of those requests, instead of making additional calls.

@gannebamm @ahmdthr if you agree I would consider these improvements before merging it. In that case the PR should be marked as Draft for the time being.

Copy link

@giohappy giohappy left a comment

Choose a reason for hiding this comment

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

@gannebamm gannebamm marked this pull request as draft July 25, 2024 13:13
@ahmdthr
Copy link
Author

ahmdthr commented Aug 22, 2024

I've tested the PR locally. The idea is good and it's really useful, but I have a few concerns:

  • every time a layer is selected in the TOC a new API request is sent to obtain the required information to enable (or not) the info button for the layer
  • the information that has been retrieved is not saved, so the same request is sent again every time a layer is selected

I think this could be improved if the following is implemented:

  • The response can be stored, somehow, inside the state of the component. This avoids making the same request for the same layer multiple times
  • But, the real point is: are these calls really needed? We already query both the map and the datasets endpoint when the map is loaded. Much information about datasets is retrieved at that time. Isn't this enough? If more attributes are required we can add them to the response of those requests, instead of making additional calls.

@gannebamm @ahmdthr if you agree I would consider these improvements before merging it. In that case the PR should be marked as Draft for the time being.

@giohappy Thank you very much for your comments. The only reason a new request was being made was because the linked resources were not included in the batch request. Now with recent changes, it is no longer necessary to sent a request to fetch linked resources. I am currently working on it and expect the final version of the PR shortly.

@ahmdthr ahmdthr marked this pull request as ready for review November 7, 2024 09:40
@ahmdthr
Copy link
Author

ahmdthr commented Nov 7, 2024

The info button for layers on the maps page now fetches all data including linked resources with a single call. Additionally, the linked resources are stored in state such that re-fetching is not necessary when re selecting one layer again.

@giohappy giohappy self-requested a review November 25, 2024 14:10
@giohappy
Copy link

@dsuren1 please do a new review.

const state = getState() || {};
const layer = getSelectedLayer(state);
const layerResourceId = layer?.extendedParams?.pk;
const layerResourceDataset = state.gnresource.data.maplayers.find(mapLayer => mapLayer.dataset.pk === parseInt(layerResourceId, 10)).dataset;
Copy link

@dsuren1 dsuren1 Nov 26, 2024

Choose a reason for hiding this comment

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

Suggested change
const layerResourceDataset = state.gnresource.data.maplayers.find(mapLayer => mapLayer.dataset.pk === parseInt(layerResourceId, 10)).dataset;
const layerResourceDataset = get(state, 'gnresource.data.maplayers', []).find(mapLayer => get(mapLayer, 'dataset.pk', '') === parseInt(layerResourceId, 10))?.dataset
  • Error when state.gnresource.data is empty. Happens probably when selecting a layer while it is still loading. Kindly check all it's references in the code

  • I see there are cases when dataset in the mapLayers is empty causing another failure even when state.gnresource.data.maplayers is not empty (ex: /map/44)

image

Copy link

@dsuren1 dsuren1 left a comment

Choose a reason for hiding this comment

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

@ahmdthr

  • It seems in some cases, an error is being generated in gnFetchMissingLayerData when selecting a layer, which is impacting further testing and review of this PR.

    image
  • Could you also please check if there is a race condition in loading the plugin? At times, the button doesn't appear on the TOC toolbar even when the required data is present.

Kindly check these comments and provide an update to the PR. Thank you!

@ahmdthr
Copy link
Author

ahmdthr commented Nov 29, 2024

@dsuren1 Thank you very much for your review. All the problems stemmed from non-existent referencing of properties, and I have fixed it in the latest commit. Please have a look at it again and let me know. Thank you.

Copy link

@dsuren1 dsuren1 left a comment

Choose a reason for hiding this comment

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

@ahmdthr
Overall the changes looks good. But I see some minor issues as mentioned below. Kindly check it out

  • Layer detail doesn't always reflect correct layer name on the detail panel

    layer_detail_sync.mp4

@@ -2330,6 +2330,150 @@
"cfg": {
"wrap": true
}
},
{
"name": "LayerDetailViewer",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"name": "LayerDetailViewer",
"mandatory": true,
"name": "LayerDetailViewer",

In order to make the plugin available by default add mandatory: true, or add it to pluginsConfig under TOC as children, so the user can select the plugin when adding viewer to map.

Currently, the plugin is not available on map with viewer existing already.

@ahmdthr
Copy link
Author

ahmdthr commented Dec 3, 2024

@ahmdthr Overall the changes looks good. But I see some minor issues as mentioned below. Kindly check it out

  • Layer detail doesn't always reflect correct layer name on the detail panel

    layer_detail_sync.mp4
    

This bug where the title of the dataset on the detail layer was not being changed was due to the fact that the "EditTtile" component was using its own state for the title, thus changing the "resource" did not update the title if the "DetailsPanel" was already inflated. It is fixed now.

@ahmdthr ahmdthr requested a review from dsuren1 December 3, 2024 14:45
@ridoo
Copy link

ridoo commented Dec 4, 2024

@dsuren1 great to see you approved the PR .. any chance to see this PR backported to 4.4.x?

@giohappy giohappy merged commit 9b1dcb4 into GeoNode:master Dec 4, 2024
2 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 4, 2024
* Added table of content to open layer details in map view. Fixes issue#1660.

* Updated to the newer version.

* Code refactor

* Fixed instability issues for info button on TOC.

* Fixed linked resources for info button on TOC and code cleanup.

* Avoided repeating calls to get layer dataset fields including linked resources for info button on TOC.

* Fixed localConfig json file for the info button.

* Reverted api changes to include linked resources, because they are already included for 'viewer_common' api_preset.

* Fixed getDatasetByPk definition.

* Fixed linting issues.

* Fixed bug where info button was misbehaving when any maplayer had no dataset.

* Fixed a bug where the title of the dataset was not changing on the details panel.

---------

Co-authored-by: Suren <[email protected]>
(cherry picked from commit 9b1dcb4)
giohappy pushed a commit that referenced this pull request Dec 4, 2024
)

* Added table of content to open layer details in map view. Fixes issue#1660.

* Updated to the newer version.

* Code refactor

* Fixed instability issues for info button on TOC.

* Fixed linked resources for info button on TOC and code cleanup.

* Avoided repeating calls to get layer dataset fields including linked resources for info button on TOC.

* Fixed localConfig json file for the info button.

* Reverted api changes to include linked resources, because they are already included for 'viewer_common' api_preset.

* Fixed getDatasetByPk definition.

* Fixed linting issues.

* Fixed bug where info button was misbehaving when any maplayer had no dataset.

* Fixed a bug where the title of the dataset was not changing on the details panel.

---------

Co-authored-by: Suren <[email protected]>
(cherry picked from commit 9b1dcb4)

Co-authored-by: ahmdthr <[email protected]>
ridoo pushed a commit to Thuenen-GeoNode-Development/geonode-mapstore-client that referenced this pull request Dec 4, 2024
…) (GeoNode#1917)

* Added table of content to open layer details in map view. Fixes issue#1660.

* Updated to the newer version.

* Code refactor

* Fixed instability issues for info button on TOC.

* Fixed linked resources for info button on TOC and code cleanup.

* Avoided repeating calls to get layer dataset fields including linked resources for info button on TOC.

* Fixed localConfig json file for the info button.

* Reverted api changes to include linked resources, because they are already included for 'viewer_common' api_preset.

* Fixed getDatasetByPk definition.

* Fixed linting issues.

* Fixed bug where info button was misbehaving when any maplayer had no dataset.

* Fixed a bug where the title of the dataset was not changing on the details panel.

---------

Co-authored-by: Suren <[email protected]>
(cherry picked from commit 9b1dcb4)

Co-authored-by: ahmdthr <[email protected]>
ridoo pushed a commit to Thuenen-GeoNode-Development/geonode-mapstore-client that referenced this pull request Dec 4, 2024
…) (GeoNode#1917)

* Added table of content to open layer details in map view. Fixes issue#1660.

* Updated to the newer version.

* Code refactor

* Fixed instability issues for info button on TOC.

* Fixed linked resources for info button on TOC and code cleanup.

* Avoided repeating calls to get layer dataset fields including linked resources for info button on TOC.

* Fixed localConfig json file for the info button.

* Reverted api changes to include linked resources, because they are already included for 'viewer_common' api_preset.

* Fixed getDatasetByPk definition.

* Fixed linting issues.

* Fixed bug where info button was misbehaving when any maplayer had no dataset.

* Fixed a bug where the title of the dataset was not changing on the details panel.

---------

Co-authored-by: Suren <[email protected]>
(cherry picked from commit 9b1dcb4)

Co-authored-by: ahmdthr <[email protected]>
ridoo added a commit to Thuenen-GeoNode-Development/geonode-mapstore-client that referenced this pull request Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No option to view detail of a layer from the map view Showing metadata per layer in map
5 participants