[Discover] Show unmapped fields by default and persist setting #91783
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
wylieconlon
added
Feature:Discover
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
wylieconlon
commented
Feb 18, 2021
| @@ -87,7 +87,7 @@ | |||
| on-remove-column="onRemoveColumn" | |||
| ></thead> | |||
| <tbody> | |||
| <tr ng-repeat="row in hits|limitTo:limit track by row._index+row._type+row._id+row._score+row._version+row._routing" | |||
| <tr ng-repeat="row in hits|limitTo:limit track by row._index+row._type+row._id+row._score+row._version+row._routing+hideUnmapped" | |||
There was a problem hiding this comment.
I added a functional test for this because it was super weird to me. The cell contents don't update unless one of these properties is updated.
There was a problem hiding this comment.
yes, this seems to be the concatenated variable Angular used to track updates, great that you've fixed it and even greater you've added a functional test for it 👍
majagrubic
reviewed
Feb 18, 2021
| */ | ||
| unmappedFieldsConfig?: { |
There was a problem hiding this comment.
Why the need to change this now? If we're planning to phase-out the unmapped fields over the next few minors, this config value will have its purpose and it doesn't really affect things atm.
majagrubic
reviewed
Feb 18, 2021
| @@ -42,6 +42,7 @@ export async function persistSavedSearch( | |||
| sort: state.sort as SortOrder[], | |||
| columns: state.columns || [], | |||
| useNewFieldsApi: false, | |||
| hideUnmappedFields: state.hideUnmapped, | |||
There was a problem hiding this comment.
Do we need to persist this setting?
|
This feels like quite a big change. I have a more minimal version of the PR that just removes the existing migration and doesn't persist anything, that we might want to consider: #91735 |
|
If we show the switch in the UI but don't persist it I think people will be confused in some cases. Let's say someone disables this flag, unmapped fields disappear, they navigate somewhere else and back and they appear again. But if we are fine with this behavior for 7.12 I wouldn't object from my side. |
kertal
reviewed
Feb 18, 2021
Comment on lines
+414
to
+422
| hideUnmapped: false, | ||
| }; | ||
| if (savedSearch.grid) { | ||
| defaultState.grid = savedSearch.grid; | ||
| } | ||
| if (savedSearch.hideChart) { | ||
| defaultState.hideChart = savedSearch.hideChart; | ||
| } | ||
| defaultState.hideUnmapped = savedSearch.hideUnmapped; |
There was a problem hiding this comment.
do you think it makes sense not to set hideUnmapped in default state unless it is explicitly set in savedSearch? This way it would also not become part of the URL unless the user changes the switch. Think the currently functional failure would be fixes without editing any expect, and we would not add a variable to URL without user action.
There was a problem hiding this comment.
Yes, I think that's reasonable.
|
@majagrubic I agree that a non-persistent setting would also make sense, but I added persistence because of the discussion yesterday about the future of this feature being off by default. |
|
As discussed we are not going to implement this as a toggle in the UI. |
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/discover/_shared_links·ts.discover app shared links shared links with state in query permalink should allow for copying the snapshot URLStandard OutStack TraceKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/discover/_shared_links·ts.discover app shared links shared links with state in query permalink should allow for copying the snapshot URLStandard OutStack TraceMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
Fixes https://github.com/elastic/kibana/pull/91783/files
In the long term, we want to hide unmapped fields by default. But in the short term, this becomes a persistent setting. This lets us make the long-term change as needed.
This PR implements two main changes:
_sourceorfieldsfields, you can now toggle unmapped fields off and persist this settingAs a side effect of this, I removed migration that @majagrubic created and replaced it with a new persistable setting.
Steps to reproduce:
dynamic: falseImportant parts to test:
Fixes #91743
Checklist