Add application.navigateToUrl core API
#67110
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pgayvallet
added
release_note:skip
|
Pinging @elastic/kibana-platform (Team:Platform) |
mshustov
approved these changes
May 20, 2020
| ); | ||
| }); | ||
| it('includes query and hash in the path', () => { | ||
| expect(parseAppUrl('/base-path/app/foo#hash/bang', basePath, apps, getOrigin)).toEqual({ |
There was a problem hiding this comment.
could you add a case for custom-bar with query? + for absolute URL
| it('returns undefined when the app is not known', () => { | ||
| expect( | ||
| parseAppUrl( | ||
| 'https://kibana.local:8080/base-path/app/non-registered', |
There was a problem hiding this comment.
could add an explicit test when navigating to other origin?
| this.navigate!(getAppUrl(availableMounters, appId, path), state); | ||
| this.currentAppId$.next(appId); | ||
| navigateToApp, | ||
| navigateToUrl: async url => { |
There was a problem hiding this comment.
not 100% sure we should, but we could make a polymorphic function
navigateTo(app: App): void
navigateTo(url: string): voidThere was a problem hiding this comment.
Not sure either. Upside from the polymorphic approach would mostly be reduced API exposure and 'smarter' API. Downside is a (little) less explicit API (navigateTo vs navigateToXXX), and a slightly more complex implementation as the signature got different number of parameters.
navigateTo(appId: string, options?: { path?: string; state?: any }): void
navigateTo(url: string): voidProbably outside of the scope of this PR as it gonna impact all current calls from plugin code, but shall I open a follow up issue to discuss this possibility?
|
|
||
| describe('navigateToUrl', () => { | ||
| it('calls `redirectTo` when the url is not parseable', async () => { | ||
| parseAppUrlMock.mockReturnValue(undefined); |
There was a problem hiding this comment.
Why not use a real call? The logic there is rather simple.
There was a problem hiding this comment.
We could remove the mock and perform a real call but as parseAppUrl is heavily tested in its own suite, I think current approach is slightly better in term of testing isolation.
joshdover
reviewed
May 20, 2020
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-oss-agent / Accessibility Tests.test/accessibility/apps/discover·ts.Discover should open context view on a docStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
pgayvallet
added a commit
to pgayvallet/kibana
that referenced
this pull request
May 25, 2020
* implement `navigateToUrl` core API * fix lint * review comments
pgayvallet
added a commit
that referenced
this pull request
May 25, 2020
* implement `navigateToUrl` core API * fix lint * review comments
jloleysens
added a commit
that referenced
this pull request
May 26, 2020
…ine-editor * 'master' of github.com:elastic/kibana: (129 commits) [Canvas] Force embeddables to refresh when renderable reevaluated (#67133) [Canvas] Better handling navigating to/from canvas (#66407) [Ingest pipelines] Fix schema validation for simulate and update routes (#67199) do not use es from setup (#67277) Auto expand replicas for event log (#67286) Observability & APM do not use elasticsearch client provided via setup contract (#67263) Fix privileges check when security is not enabled (#67308) add IIS home (#66918) [ML] Adding additional job service endpoint tests (#66892) [Ingest Manager] Update fleet internal doc with latest flags (#67193) [Discover] Deangularize the loading spinner (#67165) Add `application.navigateToUrl` core API (#67110) Improve indexpattern without timefield functional test (#67031) KibanaContext in index pattern managment ui (#66985) Fix Azure metrics tutorial inside the App Home/ Add data area (#66901) add azure logs home (#66910) fix: rum agent should work correctly on new platform (#67037) [test_utils/Testbed] Move to src/test_utils folder (OSS) (#66898) only block registration when appRoute contains the exact basePath (#67125) Changed actions API endpoints urls to follow Kibana STYLEGUIDE (#65936) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Add a new navigateToUrl API to ApplicationStart to allow navigating to a url that can either be internal or external. internal urls will be handled using navigateToApp, and external urls will be handled via location.assign
Extracted from #66293 to avoid blocking #58751 which is also going to need this API.
Checklist