-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Discover Tabs] Extend the discover locator to allow opening in a new tab #234912
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
Changes from 14 commits
efc8993
708f08e
42f403d
ea68b9e
449a634
da0f12f
cfb3ed6
a909cc8
73da781
be766b1
96a2dc8
17f6970
00add5c
3936eb8
4785f72
6bdab7e
bdaf825
6256d4d
1f9be33
1d268cc
33a1964
27766ef
1f6aa3d
38d9c42
0dec342
1808017
db8bafe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,8 @@ export const getShareAppMenuItem = ({ | |
| timeRange, | ||
| refreshInterval, | ||
| }; | ||
|
|
||
| // TODO: for a persisted saved search, add the current tab ID to the params | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably worth creating an issue for this one if we don't do it in this PR, since we should do it for 9.2. Could be a good smaller task for one of the newer team members.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created #235320 |
||
| const relativeUrl = locator.getRedirectUrl(params); | ||
|
|
||
| // This logic is duplicated from `relativeToAbsolute` (for bundle size reasons). Ultimately, this should be | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,11 @@ import { | |
| selectInitialUnifiedHistogramLayoutPropsMap, | ||
| selectTabRuntimeInternalState, | ||
| } from '../runtime_state'; | ||
| import { APP_STATE_URL_KEY, GLOBAL_STATE_URL_KEY } from '../../../../../../common/constants'; | ||
| import { | ||
| APP_STATE_URL_KEY, | ||
| GLOBAL_STATE_URL_KEY, | ||
| NEW_TAB_ID, | ||
| } from '../../../../../../common/constants'; | ||
| import type { DiscoverAppState } from '../../discover_app_state_container'; | ||
| import { createInternalStateAsyncThunk, createTabItem } from '../utils'; | ||
| import { setBreadcrumbs } from '../../../../../utils/breadcrumbs'; | ||
|
|
@@ -113,6 +117,21 @@ export const updateTabs: InternalStateThunkActionCreator<[TabbedContentState], P | |
| }; | ||
|
|
||
| if (!existingTab) { | ||
| tab.initialAppState = | ||
| 'initialAppState' in item | ||
| ? cloneDeep(item.initialAppState as TabState['initialAppState']) | ||
| : tab.initialAppState; | ||
|
|
||
| tab.globalState = | ||
| 'globalState' in item | ||
| ? cloneDeep(item.globalState as TabState['globalState']) | ||
| : tab.globalState; | ||
|
|
||
| tab.dataRequestParams = | ||
| 'dataRequestParams' in item | ||
| ? (item.dataRequestParams as TabState['dataRequestParams']) | ||
| : tab.dataRequestParams; | ||
davismcphee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (item.duplicatedFromId) { | ||
| // the new tab was created by duplicating an existing tab | ||
| const existingTabToDuplicateFrom = selectTab(currentState, item.duplicatedFromId); | ||
|
|
@@ -288,7 +307,7 @@ export const restoreTab: InternalStateThunkActionCreator<[{ restoreTabId: string | |
| (dispatch, getState) => { | ||
| const currentState = getState(); | ||
|
|
||
jughosta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (restoreTabId === currentState.tabs.unsafeCurrentId) { | ||
| if (restoreTabId === currentState.tabs.unsafeCurrentId || restoreTabId === NEW_TAB_ID) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -318,6 +337,46 @@ export const restoreTab: InternalStateThunkActionCreator<[{ restoreTabId: string | |
| ); | ||
| }; | ||
|
|
||
| export const openInNewTab: InternalStateThunkActionCreator< | ||
| [ | ||
| { | ||
| tabLabel?: string; | ||
| appState?: TabState['initialAppState']; | ||
| globalState?: TabState['globalState']; | ||
| searchSessionId?: string; | ||
| } | ||
| ] | ||
| > = | ||
| ({ tabLabel, appState, globalState, searchSessionId }) => | ||
| (dispatch, getState) => { | ||
| const initialAppState = appState ? cloneDeep(appState) : undefined; | ||
| const initialGlobalState = globalState ? cloneDeep(globalState) : {}; | ||
| const currentState = getState(); | ||
| const currentTabs = selectAllTabs(currentState); | ||
|
|
||
| const newDefaultTab: TabState = { | ||
| ...DEFAULT_TAB_STATE, | ||
| ...createTabItem(currentTabs), | ||
| initialAppState, | ||
| globalState: initialGlobalState, | ||
| }; | ||
|
|
||
| if (tabLabel) { | ||
| newDefaultTab.label = tabLabel; | ||
| } | ||
|
|
||
| if (searchSessionId) { | ||
| newDefaultTab.dataRequestParams = { | ||
| ...newDefaultTab.dataRequestParams, | ||
| searchSessionId, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would still need additional handling to sync it to the search session manager. |
||
| }; | ||
| } | ||
|
|
||
| return dispatch( | ||
| updateTabs({ items: [...currentTabs, newDefaultTab], selectedItem: newDefaultTab }) | ||
| ); | ||
| }; | ||
|
|
||
| export const disconnectTab: InternalStateThunkActionCreator<[TabActionPayload]> = | ||
| ({ tabId }) => | ||
| (_, __, { runtimeStateManager }) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we need to check both feature flags for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexGPlay Good question! How would you recommend to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was thinking that maybe the tabs one is enough since it's the feature this is truly targeting, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 1f6aa3d