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.

Sign up for GitHub

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

Jump to bottom

[Canvas] Fix nav link behavior in Canvas #65590

Merged
merged 7 commits into from
May 7, 2020
Merged

[Canvas] Fix nav link behavior in Canvas #65590

merged 7 commits into from
May 7, 2020

Conversation

poffdeluxe
Copy link
Contributor

@poffdeluxe poffdeluxe commented May 6, 2020 •
edited
Loading

Summary

This adds back in nav link tracking for the Canvas app so that we can track what path to return to when returning to Canvas from another app. This works across NP and legacy apps, too, since the nav link is set when Canvas' setup() method is called

@poffdeluxe poffdeluxe added impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0 review labels May 6, 2020
@poffdeluxe poffdeluxe marked this pull request as ready for review May 6, 2020 23:41
@poffdeluxe poffdeluxe requested a review from a team as a code owner May 6, 2020 23:41
ThomThomson
ThomThomson approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

LGTM
Tested locally in chrome and safari.
in Lens and Visualize after redirect from Canvas, 'Save as' and 'Save and return' now return to the correct workpad and page!

crob611
crob611 suggested changes May 7, 2020
View reviewed changes
Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks good and works as expected. Nice Work!

I think let's clean up the storage stuff though and then it'll be good to go.

x-pack/plugins/canvas/public/components/app/index.js Show resolved Hide resolved
x-pack/plugins/canvas/public/application.tsx Outdated
@@ -88,9 +89,10 @@ export const initializeCanvas = async (
coreStart: CoreStart,
setupPlugins: CanvasSetupDeps,
startPlugins: CanvasStartDeps,
registries: SetupRegistries
registries: SetupRegistries,
navUpdater: BehaviorSubject<AppUpdater>
Copy link
Contributor

Choose a reason for hiding this comment

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

Call this appUpdater instead of navUpdater?

x-pack/plugins/canvas/public/plugin.tsx Outdated
@@ -68,20 +72,36 @@ export class CanvasPlugin

this.srcPlugin.setup(core, { canvas: canvasApi });

// Set the nav link to the last saved url if we have one in storage
const lastUrl = sessionStorage.getItem(SESSIONSTORAGE_LASTPATH);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a wrapper around Storage that we use in other places src/plugins/kibana_utils/public.

I think it's just about parsing/stringifying if we're putting in JSON. Right now it's only in clipboard I think, but maybe we move the setting/getting of storage into a lib and that can be shared elsewhere.

crob611
crob611 approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks great!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@poffdeluxe poffdeluxe added the bug Fixes for quality problems that affect the customer experience label May 7, 2020
@poffdeluxe poffdeluxe merged commit 9f11d21 into elastic:master May 7, 2020
@poffdeluxe poffdeluxe deleted the corey-nav-link-changes branch May 7, 2020 19:54
@poffdeluxe poffdeluxe mentioned this pull request May 7, 2020
Merged
poffdeluxe added a commit that referenced this pull request May 7, 2020
@poffdeluxe
[Canvas] Fix nav link behavior in Canvas (#65590) (#65763)
ba6037e 
* wip

* Storing last page for canvas in session storage

* Fix bad path

* Fix bad merge

* Cleanup and adding some types

* Fixing types

* PR feedback and storage refactor

Co-authored-by: Corey Robertson <corey.robertson@elastic.co>

Co-authored-by: Corey Robertson <corey.robertson@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 8, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into pipeline-edit…
c7fe2db 
…or-part-mvp-2

* 'master' of github.com:elastic/kibana: (259 commits)
  SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id (elastic#65150)
  Drilldown count tooltip (elastic#65105)
  plugins logs start with "plugins." prefix (elastic#65710)
  [ML] Fix pagination reset on search query update. (elastic#65668)
  [SIEM] Add types to the mappings objects so extra keys cannot be introduced
  [apm] Update machine learning flyout and service maps docs (elastic#65517)
  change api endpoint and throw error (elastic#65790)
  [Maps] remove SLA percentage metric (elastic#65718)
  [Reporting] APM integration for baseline performance measurements (elastic#59967)
  fix(NA): noParse regex for windows on kbn optimizer (elastic#65755)
  [ML] DFA: ensure at least one field is included in analysis before job can be created (elastic#65320)
  [Data plugin] cleanup - remove unused getRoutes / routes from indexPattern object (elastic#65683)
  Removed skip to enable test. (elastic#65575)
  [Lens] Type safe migrations (elastic#65576)
  [Canvas] Fix nav link behavior in Canvas  (elastic#65590)
  [Event log] Fix flaky test (elastic#65658)
  [Alerting] changes preconfigured actions config from array to object (elastic#65397)
  remove immediate functions from esqueue worker cycles (elastic#65375)
  [Metrics UI] Fix isAbove to only display when threshold set (elastic#65540)
  draft search profiler accessibility tests (elastic#62357)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
This was referenced May 8, 2020
Merged
Closed
poffdeluxe added a commit that referenced this pull request May 8, 2020
@poffdeluxe
[Canvas] Fix nav link behavior in Canvas (#65590) (#65850)
6115351 
* wip

* Storing last page for canvas in session storage

* Fix bad path

* Fix bad merge

* Cleanup and adding some types

* Fixing types

* PR feedback and storage refactor

Co-authored-by: Corey Robertson <corey.robertson@elastic.co>

Co-authored-by: Corey Robertson <corey.robertson@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomThomson ThomThomson ThomThomson approved these changes

@crob611 crob611 crob611 approved these changes

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@poffdeluxe @kibanamachine @crob611 @ThomThomson