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.

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

[Discover] Navigating from Field statistics tab to text based languages should return to the documents view #152572

Merged
merged 6 commits into from
Apr 27, 2023

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Mar 2, 2023

Closes #152485
Would be better to override this somewhere in app state management logic but I could not find a right place for it. Done

Current changes make sure that for text-based queries only grid view is possible (both on Discover and as embeddable) and the app state will be updated accordingly.

@jughosta jughosta closed this Apr 5, 2023
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

@jughosta wanna iterate on that? you seem to have solved the issue in the embeddable, to solve in in the state, it should work like this:

const nextState = {
...(addDataViewToState && { index: dataViewObj.id }),
...(addColumnsToState && { columns: nextColumns }),
};

  const nextState = {
          ...(addDataViewToState && { index: dataViewObj.id }),
          ...(addColumnsToState && { columns: nextColumns }),
          ...(viewMode === VIEW_MODE.AGGREGATED_LEVEL && { viewMode: VIEW_MODE.DOCUMENT_LEVEL }),
        };

# Conflicts:
#	src/plugins/discover/public/application/main/components/layout/discover_main_content.tsx
@jughosta jughosta reopened this Apr 26, 2023
@jughosta jughosta self-assigned this Apr 26, 2023
@jughosta jughosta added Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Apr 26, 2023
@jughosta
Copy link
Contributor Author

@kertal Thanks! The suggestion works!

@kertal
Copy link
Member

kertal commented Apr 26, 2023

Great, glad I could help!

@jughosta jughosta marked this pull request as ready for review April 26, 2023 15:05
@jughosta jughosta requested a review from a team as a code owner April 26, 2023 15:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

Comment on lines 174 to 175
const recordRawType = getRawRecordType(query);
return recordRawType === RecordRawType.PLAIN;
Copy link
Member

Choose a reason for hiding this comment

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

isPlainRecord would also be an option here

export function isPlainRecord(query?: Query | AggregateQuery): query is AggregateQuery {

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Thx for brining this over the finishing line! 👍 Code LGTM
One last thing to consider, given a saved search is already persisted with the aggregated view , then we might take this into consideration when building the initial state

It could be done here

if (savedSearch.viewMode) {
defaultState.viewMode = savedSearch.viewMode;
}
if (savedSearch.hideAggregatedPreview) {

Like this:

 if (savedSearch.viewMode && !isPlainRecord(query)) {
    defaultState.viewMode = savedSearch.viewMode;
  }

wdyt?

@kertal
Copy link
Member

kertal commented Apr 27, 2023

We might also consider to rename isPlainRecord ... or to create an alias function ... isTextBasedLanguage ... I always struggle to remember the plain name ... and I think, I was the one suggesting this name? 😄

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 468 470 +2

Async chunks

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

id before after diff
discover 432.3KB 431.6KB -679.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
discover 28.0KB 28.0KB +37.0B
Unknown metric groups

async chunk count

id before after diff
discover 9 10 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 399 402 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 479 482 +3
total +5

History

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

cc @jughosta

@jughosta
Copy link
Contributor Author

@kertal Updated

Thanks for the feedback!

@jughosta jughosta requested a review from kertal April 27, 2023 13:00
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Thx for fixing it, adding tests and making the code prettier 🙇 Tested locally, works as expected. No more desperate people in SQL trying to leave field statistics!

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 27, 2023
…es should return to the documents view (elastic#152572)

Closes elastic#152485
~Would be better to override this somewhere in app state management
logic but I could not find a right place for it.~ Done

Current changes make sure that for text-based queries only grid view is
possible (both on Discover and as embeddable) and the app state will be
updated accordingly.

(cherry picked from commit f528b34)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Apr 27, 2023
…anguages should return to the documents view (#152572) (#156010)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Discover] Navigating from Field statistics tab to text based
languages should return to the documents view
(#152572)](#152572)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Julia
Rechkunova","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-04-27T13:43:32Z","message":"[Discover]
Navigating from Field statistics tab to text based languages should
return to the documents view (#152572)\n\nCloses
https://github.com/elastic/kibana/issues/152485\r\n~Would be better to
override this somewhere in app state management\r\nlogic but I could not
find a right place for it.~ Done\r\n\r\nCurrent changes make sure that
for text-based queries only grid view is\r\npossible (both on Discover
and as embeddable) and the app state will be\r\nupdated
accordingly.","sha":"f528b34512171455a70c89c13d012feca4f67f61","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:fix","Team:DataDiscovery","backport:prev-minor","v8.9.0"],"number":152572,"url":"https://github.com/elastic/kibana/pull/152572","mergeCommit":{"message":"[Discover]
Navigating from Field statistics tab to text based languages should
return to the documents view (#152572)\n\nCloses
https://github.com/elastic/kibana/issues/152485\r\n~Would be better to
override this somewhere in app state management\r\nlogic but I could not
find a right place for it.~ Done\r\n\r\nCurrent changes make sure that
for text-based queries only grid view is\r\npossible (both on Discover
and as embeddable) and the app state will be\r\nupdated
accordingly.","sha":"f528b34512171455a70c89c13d012feca4f67f61"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/152572","number":152572,"mergeCommit":{"message":"[Discover]
Navigating from Field statistics tab to text based languages should
return to the documents view (#152572)\n\nCloses
https://github.com/elastic/kibana/issues/152485\r\n~Would be better to
override this somewhere in app state management\r\nlogic but I could not
find a right place for it.~ Done\r\n\r\nCurrent changes make sure that
for text-based queries only grid view is\r\npossible (both on Discover
and as embeddable) and the app state will be\r\nupdated
accordingly.","sha":"f528b34512171455a70c89c13d012feca4f67f61"}}]}]
BACKPORT-->

Co-authored-by: Julia Rechkunova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.8.0 v8.9.0
Projects
None yet
5 participants