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] Fixed tooltips for text and keyword fields displaying 'Unknown field' in expanded document #133536

Merged
merged 8 commits into from
Jun 10, 2022

Conversation

davismcphee
Copy link
Contributor

@davismcphee davismcphee commented Jun 3, 2022

Summary

This PR fixes an issue where tooltips/mouseover text on the expanded document shows 'Unknown field' for keyword or text fields.

The issue stems from the field list and expanded document flyout using two different functions to get the field name. The functions seemed to be clones of each other so I removed the broken one and moved the working one to what looks like a common utils folder so both areas can share it.

text_field_name

keyword_field_name

Fixes #131788.

Checklist

For maintainers

@davismcphee davismcphee added bug Fixes for quality problems that affect the customer experience Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jun 3, 2022
@davismcphee davismcphee requested a review from a team as a code owner June 3, 2022 15:05
@elasticmachine
Copy link
Contributor

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

@davismcphee davismcphee marked this pull request as draft June 3, 2022 19:01
@davismcphee davismcphee self-assigned this Jun 3, 2022
@davismcphee davismcphee added release_note:fix auto-backport Deprecated - use backport:version if exact versions are needed v8.3.0 v8.4.0 v8.2.3 and removed bug Fixes for quality problems that affect the customer experience labels Jun 3, 2022
@davismcphee davismcphee force-pushed the fix-text-keyword-field-tooltips branch from 4445e44 to 723b332 Compare June 6, 2022 14:06
@kertal
Copy link
Member

kertal commented Jun 7, 2022

@elasticmachine merge upstream

@@ -29,7 +29,7 @@ import type { DataViewField, DataView } from '@kbn/data-views-plugin/public';
import { getTypeForFieldIcon } from '../../../../utils/get_type_for_field_icon';
import { DiscoverFieldDetails } from './discover_field_details';
import { FieldDetails } from './types';
import { getFieldTypeName } from './lib/get_field_type_name';
Copy link
Member

Choose a reason for hiding this comment

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

Thx a lot for cleaning this up and removing duplicated code 👍 . You can flag the PR ready for review. One thing you could add on top of it, could be a Jest test for e.g. discover_field.tsx that would catch the error. Or a basic one for get_field_type_name.ts

Copy link
Contributor Author

@davismcphee davismcphee Jun 8, 2022

Choose a reason for hiding this comment

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

Tests added for get_field_type_name.ts

@davismcphee davismcphee marked this pull request as ready for review June 8, 2022 17:10
return i18n.translate('discover.fieldNameIcons.versionFieldAriaLabel', {
defaultMessage: 'Version field',
});
default:
return i18n.translate('discover.fieldNameIcons.unknownFieldAriaLabel', {
defaultMessage: 'Unknown field',
defaultMessage: UNKNOWN_FIELD_TYPE_MESSAGE,
Copy link
Member

Choose a reason for hiding this comment

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

this is breaking the build. when you revert the change your tests would no longer work. So you could rewrite the test to first get the default message getFieldTypeName(), and check that all known fieldtypes return different values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Broken tests should now be fixed

@davismcphee davismcphee marked this pull request as draft June 9, 2022 19:01
@davismcphee davismcphee marked this pull request as ready for review June 9, 2022 20:38
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.

LGTM, didn't test again, great to have more test coverage, Congrats and welcome to the Discover side of Kibana 🥳

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #7 / analytics instrumented events from the browser General "click" "before each" hook for "should emit a "click" event"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 548 547 -1

Async chunks

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

id before after diff
discover 448.0KB 449.3KB +1.3KB

Page load bundle

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

id before after diff
discover 40.6KB 40.6KB +5.0B

History

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

cc @davismcphee

@davismcphee davismcphee merged commit 3e3ee3e into elastic:main Jun 10, 2022
@davismcphee davismcphee deleted the fix-text-keyword-field-tooltips branch June 10, 2022 00:24
kibanamachine pushed a commit that referenced this pull request Jun 10, 2022
…nown field' in expanded document (#133536)

* [Discover] Fixed tooltips for text and keyword fields displaying 'Unknown field'

* [Discover] Added tests for getFieldTypeName function

* [Discover] Fixing issue where i18n defaultMessage was a variable instead of a constant string

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 3e3ee3e)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
8.3
8.2 Backport failed because of merge conflicts

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

Manual backport

To create the backport manually run:

node scripts/backport --pr 133536

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jun 10, 2022
…nown field' in expanded document (#133536) (#134153)

* [Discover] Fixed tooltips for text and keyword fields displaying 'Unknown field'

* [Discover] Added tests for getFieldTypeName function

* [Discover] Fixing issue where i18n defaultMessage was a variable instead of a constant string

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 3e3ee3e)

Co-authored-by: Davis McPhee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed 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.2.3 v8.3.0 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Expanded document view/Document Grid - Mouseover text not working for text and keyword fields
5 participants