Skip to content

[Maps] Fix saved object (map) switching issue#112001

Merged
kindsun merged 10 commits intoelastic:masterfrom
kindsun:fix-issue-switching-maps
Sep 15, 2021
Merged

[Maps] Fix saved object (map) switching issue#112001
kindsun merged 10 commits intoelastic:masterfrom
kindsun:fix-issue-switching-maps

Conversation

@kindsun
Copy link
Copy Markdown
Contributor

@kindsun kindsun commented Sep 13, 2021

Currently trying to switch between recently viewed maps doesn't actually change maps:

Peek 2021-09-13 10-54

The URL changes and a routing event is detected by the Maps app, but the saved object isn't actually loaded and no state is applied. This was discovered while working on #111195 which requires navigation between saved objects in case of a conflict.

This PR just forces react to generate a new map page if a new map saved object id is passed through.

Peek 2021-09-13 11-22

@kindsun kindsun added Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.15.1 v7.16.0 v8.0.0 labels Sep 13, 2021
@kindsun kindsun marked this pull request as ready for review September 13, 2021 17:40
@kindsun kindsun requested a review from a team as a code owner September 13, 2021 17:40
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@kindsun
Copy link
Copy Markdown
Contributor Author

kindsun commented Sep 15, 2021

@elasticmachine merge upstream

@nreese
Copy link
Copy Markdown
Contributor

nreese commented Sep 15, 2021

Thanks for finding this issue and putting up a fix. There is actually a much easier solution that will avoid adding additional componentDidUpdate and getDerivedStateFromProps checks.

In https://github.com/elastic/kibana/blob/master/x-pack/plugins/maps/public/render_app.tsx#L95, just add the line key={routeProps.match.params.savedMapId ? routeProps.match.params.savedMapId : 'new'}. That will force react to create a new instance of MapPage when savedMapId changes.

    <MapPage
        key={routeProps.match.params.savedMapId ? routeProps.match.params.savedMapId : 'new'}
        mapEmbeddableInput={mapEmbeddableInput}
        embeddableId={embeddableId}
        onAppLeave={onAppLeave}
        setHeaderActionMenu={setHeaderActionMenu}
        stateTransfer={stateTransfer}
        originatingApp={originatingApp}
      />

@kindsun
Copy link
Copy Markdown
Contributor Author

kindsun commented Sep 15, 2021

Thanks for finding this issue and putting up a fix. There is actually a much easier solution that will avoid adding additional componentDidUpdate and getDerivedStateFromProps checks.

In master/x-pack/plugins/maps/public/render_app.tsx#L95, just add the line key={routeProps.match.params.savedMapId ? routeProps.match.params.savedMapId : 'new'}. That will force react to create a new instance of MapPage when savedMapId changes.

    <MapPage
        key={routeProps.match.params.savedMapId ? routeProps.match.params.savedMapId : 'new'}
        mapEmbeddableInput={mapEmbeddableInput}
        embeddableId={embeddableId}
        onAppLeave={onAppLeave}
        setHeaderActionMenu={setHeaderActionMenu}
        stateTransfer={stateTransfer}
        originatingApp={originatingApp}
      />

👍 Easier sounds good, thanks! I'll give this a shot

@kindsun kindsun force-pushed the fix-issue-switching-maps branch from db067f2 to 62b8d65 Compare September 15, 2021 15:55
Copy link
Copy Markdown
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM
code review, tested in chrome.

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 3.2MB 3.2MB +80.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kindsun kindsun added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 15, 2021
@kindsun kindsun merged commit 4309356 into elastic:master Sep 15, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 15, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 15, 2021
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Backport successful

Status Branch Result
7.x
7.15

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 15, 2021
* Add key to MapPage

Co-authored-by: Aaron Caldwell <aaron.caldwell@elastic.co>
kibanamachine added a commit that referenced this pull request Sep 15, 2021
* Add key to MapPage

Co-authored-by: Aaron Caldwell <aaron.caldwell@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed release_note:fix Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.15.1 v7.16.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants