-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Lens] Provide single-value functions to show the "Last" value of some field #83437
Conversation
@elasticmachine merge upstream |
Pinging @elastic/kibana-app (Team:KibanaApp) |
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.
Just a minor a11y question but want an actual designer to take a look at my Sass comment but otherwise LGTM
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.scss
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Outdated
Show resolved
Hide resolved
Code looks good 👍 , just two minor changes proposed. Will test locally next 🚀 |
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.
As this is a new place to store a field reference, we should
- add it to the validation logic in
x-pack/plugins/lens/public/indexpattern_datasource/utils.ts#getInvalidLayers
- Handle missing fields in the UI similar to how it's done for invalid
sourceFields
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx
Show resolved
Hide resolved
@elasticmachine merge upstream |
…anel/dimension_editor.tsx
@elasticmachine merge upstream |
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.
This looks great! Left some minor comments
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts
Outdated
Show resolved
Hide resolved
…st_value # Conflicts: # x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts
@elasticmachine merge upstream |
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.
Tested and LGTM
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Outdated
Show resolved
Hide resolved
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.
This is looking great, @mbondyra! Left a few comments for your review.
...k/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.tsx
Outdated
Show resolved
Hide resolved
...k/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.tsx
Outdated
Show resolved
Hide resolved
...k/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.tsx
Outdated
Show resolved
Hide resolved
...k/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx
Outdated
Show resolved
Hide resolved
…t_value # Conflicts: # x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/count.tsx # x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts # x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/metrics.tsx
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.
Thank you for making those previous changes. I've left a few more minor stylistic change requests/comments, but I'll approve now so as not to hold you up any further.
...k/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.tsx
Show resolved
Hide resolved
.../plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.scss
Outdated
Show resolved
Hide resolved
.../plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/warnings_popover.scss
Outdated
Show resolved
Hide resolved
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.
The functionality is working, including error states- most of my comments are about the form design. I'm hoping we can make a more user-friendly form design like we talked about, by removing the words "sort order" from the UI.
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
Outdated
Show resolved
Hide resolved
})} | ||
isInvalid={isSortFieldInvalid} | ||
> | ||
<EuiComboBox |
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 can't blur this combobox on click outside. Is it configured differently than usual? Something seems wrong.
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 cannot reproduce it. Can you send me a video or let's talk during sync.
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 haven't re-tested it, but I'm approving now that sort order is removed. The form UI issue is something I'm not just seeing on your PR, so let's deal with it separately.
…t_value # Conflicts: # x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx
f072f69
to
51e0830
Compare
…alue of some field (elastic#83437)
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
History
To update your PR or re-run it, just comment with: |
Summary
Fixes #55895 and #83655
This PR adds new type of metric operation
last value
– it has the same behavior as Visualize 'top_hits' but with some limitations prioritizing user experience. Here's the behavior description:Here's the example screenshots:
When there's no timefield:
When the time field was removed from index and there's invalid reference:
I changed the messaging in error handling – now we don't use names of fields, but the label of column. I think it's a change for better because it's much easier for the user to identify the place where the reference has to be fixed.
With numeric arrays for metric on pie (or y-accessors for xy chart)
Checklist
Delete any items that are not applicable to this PR.