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

[DataViewField] Allow to add a custom description for data view fields #168577

Merged
merged 42 commits into from
Mar 19, 2024

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Oct 11, 2023

Summary

This PR extends Data View Field flyout with a new textarea to enter and save a custom field description. This description will be shown in a field popover for Discover sidebar, in Doc Viewer and also on Data View management page. Current limit for the custom description is 300.

When creating/editing a field:
Screenshot 2024-03-07 at 18 59 24

In the field popover(truncated):
Screenshot 2024-03-07 at 18 56 52

In the field popover(expanded):
Screenshot 2024-03-07 at 18 57 00

In Doc Viewer popover(always expanded):
Screenshot 2024-03-07 at 18 57 21

On Data View Management page(truncated):
Screenshot 2024-03-07 at 18 57 42

Initial implementation examples

Oct-11-2023 11-45-45

Screenshot 2023-10-11 at 11 52 22 Screenshot 2023-10-11 at 11 46 44 Screenshot 2023-10-11 at 11 46 29 Screenshot 2023-10-11 at 11 47 15 Screenshot 2023-10-11 at 12 05 29

Checklist

@jughosta jughosta added release_note:enhancement Feature:Data Views Data Views code and UI - index patterns before 8.0 backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Oct 11, 2023
@jughosta jughosta self-assigned this Oct 11, 2023
@mattkime
Copy link
Contributor

mattkime commented Oct 11, 2023

Suggestion - make the description field a single row and place it beneath the name field. No toggle needed.

I'd prefer to put the description inside the field list hover directly instead of having a hover in the hover. There should be a way of truncating it or perhaps having an expand and collapse.

Perhaps limit it to 255 characters.

It would be nice if the field search could also search descriptions, but that might be a follow up.

@ninoslavmiskovic
Copy link
Contributor

@mattkime some good ideas 💡

I would be hesitant to include it in the field list to avoid using up that much space. Also might be look scattered if some fields have descriptions and others don't.

What we could investigate perhaps is adding the "I" icon on the fields that have descriptions on the "main" list and not just the popover.

I agree on placing it near the name (like mentioned on slack).

++ on the limit for 255 characters.

@andreadelrio could charm in.

BTW - we are planning some work on the field list for the near term, where this could be relevant to include.

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Looks and works great! I think this would definitely add value for users and is worth proceeding with. In the future we might be able to enhance the functionality with things like automatic descriptions for ECS fields, etc.

Suggestion - make the description field a single row and place it beneath the name field. No toggle needed.

I agree with this. The toggle feels unnecessary already so avoiding another one would be good. If we group them under one toggle, maybe we should rename it to something like "Set details".

I'd prefer to put the description inside the field list hover directly instead of having a hover in the hover. There should be a way of truncating it or perhaps having an expand and collapse.

I also think this could be a good idea. I took a quick attempt at it, were you thinking something along these lines?
combined

What we could investigate perhaps is adding the "I" icon on the fields that have descriptions on the "main" list and not just the popover.

Personally I'd recommend against adding anything to the field list and keeping it only in the popover since the field list can already become crowded with long names and multiple icons.

@teresaalvarezsoler
Copy link

hey team, this is a great feature, are we planning to show these field description also in Lens? :)

@davismcphee
Copy link
Contributor

@teresaalvarezsoler Since it's still in a PoC state I can't say for sure what the final experience will look like, but if we add the descriptions to the field list popover or somewhere else in the field list, then it should also be supported for Lens by default 🙂

@jughosta
Copy link
Contributor Author

Hi team,
Pushed some updates and here is the current state:

Screenshot 2023-12-22 at 17 22 33 Screenshot 2023-12-22 at 17 22 18 Screenshot 2023-12-22 at 17 26 56 Screenshot 2023-12-22 at 17 22 06

I think it's important to have a textarea for the description and I set the limit to 300 so users can easily enter a longer description for the field. I like having this textarea behind a switch toggle so it does not take much space in the flyout by default.

Let me know if it looks good or more modifications needed before I add tests.

cc @andreadelrio

@jughosta
Copy link
Contributor Author

/ci

@jughosta
Copy link
Contributor Author

/ci

@jughosta
Copy link
Contributor Author

/ci

@jughosta jughosta marked this pull request as ready for review March 11, 2024 18:37
@jughosta jughosta requested review from a team as code owners March 11, 2024 18:37
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

limits.yml

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Code look good and works well. Nice work!

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Code changes look good, and I tested locally and it seems to be working well! Great work and LGTM 👍

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Vis team changes LGTM, tested the Lens field list and everything works as expected

jughosta and others added 2 commits March 15, 2024 13:17
…tor/field_detail.tsx

Co-authored-by: amyjtechwriter <61687663+amyjtechwriter@users.noreply.github.com>
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.

Awesome work, another ON week win 🏅 ! One question, should we render newlines? in this example I've added newlines in the editor with every number, the question is if this should also be rendered in the UI this way
Bildschirmfoto 2024-03-18 um 15 27 23

@kertal
Copy link
Member

kertal commented Mar 19, 2024

one note from my end, multiline support shouldn't block from moving forward here, If it makes sense it could be done in a follow up. Something like this could also lead to edge cases, what would happen if there are lots of multilines like
1\n2\n3\n....

Copy link
Contributor

@ersin-erdal ersin-erdal left a comment

Choose a reason for hiding this comment

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

ResponseOps (Alerting) changes LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 440 464 +24
apm 1626 1650 +24
cloudSecurityPosture 410 414 +4
dataViewFieldEditor 160 161 +1
dataViewManagement 195 219 +24
dataVisualizer 628 632 +4
discover 750 754 +4
eventAnnotationListing 470 474 +4
lens 1347 1351 +4
logsExplorer 678 682 +4
slo 610 614 +4
unifiedDocViewer 114 118 +4
unifiedHistogram 123 127 +4
total +109

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/field-utils 30 40 +10

Async chunks

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

id before after diff
aiops 403.2KB 404.5KB +1.3KB
apm 3.2MB 3.2MB +1.3KB
dataViewFieldEditor 173.6KB 175.2KB +1.6KB
dataViewManagement 137.8KB 139.2KB +1.3KB
discover 581.8KB 583.1KB +1.3KB
lens 1.4MB 1.4MB +1.4KB
slo 614.4KB 615.8KB +1.3KB
unifiedDocViewer 58.4KB 60.3KB +1.9KB
total +11.5KB

Page load bundle

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

id before after diff
dataViewFieldEditor 25.1KB 25.3KB +289.0B
dataViews 49.0KB 49.8KB +868.0B
slo 20.7KB 20.7KB +1.0B
total +1.1KB
Unknown metric groups

API count

id before after diff
@kbn/field-utils 38 48 +10
data 3249 3257 +8
dataViews 955 970 +15
total +33

History

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

cc @jughosta

@jughosta jughosta merged commit fc2c389 into elastic:main Mar 19, 2024
34 checks passed
@jughosta jughosta deleted the custom-field-description branch March 19, 2024 16:31
@alexfrancoeur
Copy link

❤️ ❤️ ❤️ ❤️

jughosta added a commit that referenced this pull request Mar 27, 2024
- Follow up for #168577

## Summary

We should probably exclude `customDescription`s from the data view
minimal spec as they tend to be long.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
jughosta added a commit that referenced this pull request Jul 11, 2024
- Closes #187075

## Summary

This PR updates data view API docs. `customDescription` was added in
#168577

---------

Co-authored-by: Lisa Cawley <lcawley@elastic.co>
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 11, 2024
- Closes elastic#187075

## Summary

This PR updates data view API docs. `customDescription` was added in
elastic#168577

---------

Co-authored-by: Lisa Cawley <lcawley@elastic.co>
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
(cherry picked from commit cd4a782)
kibanamachine added a commit that referenced this pull request Jul 11, 2024
# Backport

This will backport the following commits from `main` to `8.15`:
- [[Discover] Update data view API docs
(#187146)](#187146)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Julia
Rechkunova","email":"julia.rechkunova@elastic.co"},"sourceCommit":{"committedDate":"2024-07-11T09:33:52Z","message":"[Discover]
Update data view API docs (#187146)\n\n- Closes
https://github.com/elastic/kibana/issues/187075\r\n\r\n##
Summary\r\n\r\nThis PR updates data view API docs. `customDescription`
was added
in\r\nhttps://github.com//pull/168577\r\n\r\n---------\r\n\r\nCo-authored-by:
Lisa Cawley <lcawley@elastic.co>\r\nCo-authored-by: Davis McPhee
<davismcphee@hotmail.com>","sha":"cd4a782cac7689b12087d2cdb0b2c9bcc676728e","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:DataDiscovery","backport:prev-minor","v8.16.0"],"title":"[Discover]
Update data view API
docs","number":187146,"url":"https://github.com/elastic/kibana/pull/187146","mergeCommit":{"message":"[Discover]
Update data view API docs (#187146)\n\n- Closes
https://github.com/elastic/kibana/issues/187075\r\n\r\n##
Summary\r\n\r\nThis PR updates data view API docs. `customDescription`
was added
in\r\nhttps://github.com//pull/168577\r\n\r\n---------\r\n\r\nCo-authored-by:
Lisa Cawley <lcawley@elastic.co>\r\nCo-authored-by: Davis McPhee
<davismcphee@hotmail.com>","sha":"cd4a782cac7689b12087d2cdb0b2c9bcc676728e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/187146","number":187146,"mergeCommit":{"message":"[Discover]
Update data view API docs (#187146)\n\n- Closes
https://github.com/elastic/kibana/issues/187075\r\n\r\n##
Summary\r\n\r\nThis PR updates data view API docs. `customDescription`
was added
in\r\nhttps://github.com//pull/168577\r\n\r\n---------\r\n\r\nCo-authored-by:
Lisa Cawley <lcawley@elastic.co>\r\nCo-authored-by: Davis McPhee
<davismcphee@hotmail.com>","sha":"cd4a782cac7689b12087d2cdb0b2c9bcc676728e"}}]}]
BACKPORT-->

Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:enhancement Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Data field descriptions