Load Lens together with kibana app #50164
Conversation
flash1293
changed the title
💔 Build Failed |
💔 Build Failed |
|
@elasticmachine merge upstream |
💔 Build Failed |
|
Making this ready for review even the build failed (error was a problem reaching github). |
flash1293
added
the
Team:Visualizations
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
|
@elasticmachine merge upstream |
💚 Build Succeeded |
💚 Build Succeeded |
| window.location.hash.replace(/^#\//, '#/lens/'); | ||
|
|
||
| // redirect | ||
| window.location.href = window.location.href.replace( |
There was a problem hiding this comment.
This code seems a bit brittle. Can we do something to avoid replacing the location in such a way?
There was a problem hiding this comment.
I see what you mean. This code is covering a very specific edge case - when a user bookmarks a Lens visualization in 7.5
As it's just here for BWC reasons I wanted to keep it as simple as possible. Do you have a suggestion for improvement?
There was a problem hiding this comment.
I agree with Maja. It'll still be hacky, but I'd be more OK with this if it was done inside a conditional, rather than 100% of the time that Lens loads up. (e.g. if we detect we're on a 7.5 URl, redirect to a 7.6 URL, etc).
There was a problem hiding this comment.
Ah, there might be a misconception. This isn't executed each time Lens loads - just if you navigate to app/lens (which can only happen if you have a bookmark stored because the Visualize dialog will point to app/kibana#/lens instead).
|
@elasticmachine merge upstream |
💚 Build Succeeded |
| window.location.hash.replace(/^#\//, '#/lens/'); | ||
|
|
||
| // redirect | ||
| window.location.href = window.location.href.replace( |
There was a problem hiding this comment.
I agree with Maja. It'll still be hacky, but I'd be more OK with this if it was done inside a conditional, rather than 100% of the time that Lens loads up. (e.g. if we detect we're on a 7.5 URl, redirect to a 7.6 URL, etc).
| @@ -26,10 +26,11 @@ export const lens: LegacyPluginInitializer = kibana => { | |||
| app: { | |||
| title: NOT_INTERNATIONALIZED_PRODUCT_NAME, | |||
| description: 'Explore and visualize data.', | |||
| main: `plugins/${PLUGIN_ID}/index`, | |||
| main: `plugins/${PLUGIN_ID}/redirect`, | |||
There was a problem hiding this comment.
It doesn't look like we have a '/redirect' route in Lens at all. I'm assuming this is a standard Kibana app thing or something? Just wanted to double check that this was an intentional change.
There was a problem hiding this comment.
This isn't a route, it's the entrypoint of the app/lens "app" which doesn't exist anymore. It points to the redirect.ts file which does the redirect to the actual app which is loaded as part of the kibana app.
All of this is an interim measure, with the new platform we can just use the application service directly.
There was a problem hiding this comment.
Overall this change is working for me, and I like that Lens is now getting a bit better integration with the rest of Kibana. I have the same concerns as others about the URL issue, but I'm not even sure we need the redirect for backwards compatibility.
Either way, this probably needs a release note that the Lens URL is changing.
| fromDate: timeDefaults.from, | ||
| toDate: timeDefaults.to, | ||
| fromDate: data.query.timefilter.timefilter.getTime().from, | ||
| toDate: data.query.timefilter.timefilter.getTime().to, |
There was a problem hiding this comment.
Nice, I like this change!
| @@ -204,6 +203,8 @@ export function App({ | |||
| trackUiEvent('app_query_change'); | |||
| } | |||
|
|
|||
| data.query.timefilter.timefilter.setTime(dateRange); | |||
There was a problem hiding this comment.
Not sure what effect this has, but is it worth looking at whether the new date range differs from the old date range? There is already a condition above
There was a problem hiding this comment.
This syncs the time range in the other direction (from within the app to the global state), but you are right, we can put it into that condition.
| ); | ||
| localApplicationService.register({ | ||
| id: 'lens', | ||
| title: 'Lens', |
There was a problem hiding this comment.
Can or should this be i18n?
There was a problem hiding this comment.
It's not really used right now, it's just there to have the same interface as the real application service. I will use the NOT_INTERNATIONALIZED_PRODUCT_NAME for now.
|
Improved the implementation of the redirect code to make it look less brittle. |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
| const prefixedHashRoute = window.location.hash.replace(/^#\//, '#/lens/'); | ||
|
|
||
| // redirect to the new lens url `app/kibana#/lens/...` | ||
| window.location.href = npSetup.core.http.basePath.prepend('/app/kibana' + prefixedHashRoute); |
There was a problem hiding this comment.
I like this a lot better.
This PR hooks the lens routes into the angular routing of Kibana app and loads it in the same bundle to prevent the page load between the apps.
This changes the URL:
app/lensbecomesapp/kibana#/lens.Old urls are redirected to the new URL scheme.
As there is no page load in between, the local state of the data plugin can be leveraged to keep filters and time range when switching between Lens visualizations and others.