-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Annotations design review #140588
[Lens] Annotations design review #140588
Conversation
b664f70
to
85d1d11
Compare
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
9edf8cd
to
5d79dfa
Compare
src/plugins/chart_expressions/expression_xy/public/components/annotations.tsx
Outdated
Show resolved
Hide resolved
8ef6a50
to
12b51c8
Compare
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 everything works well, LGTM
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.
Thanks for making those changes, @mbondyra. This looks really great! Thanks for going the extra mile to do that quick pass on tooltips too. I like the changes you've made to them. Any final recommendations to their styling has been added as a code comment below.
Otherwise, here's my list of comments for your review:
-
It looks like we switched back to using the popover-based query input, rather than the previous iteration that used an inline query input that automatically increased its height on focus. Out of curiosity, what was the reason for the switch? I personally liked the version without the popover.
-
With the current popover-based query input, I noticed that there is no autocomplete that appears when focusing or typing in the input. I assume this is a bug, as it used to appear in other areas of Lens as well (such as the "Filter" quick function), but no longer appears to do so now.
-
The padding and margining seem a bit off in the "Show additional fields" container (compared to the other DND field containers, like multi-field top values). There should be 8px of padding on the container and 8px of margining in between the field rows. Would that be possible to fix?
-
The green drop zone that appears during drag events for the "Show additional fields" container doesn't appear to have the rounded corners that match the ones shown on the originating gray container. Possible to add?
-
When there is only one field in "Show additional fields" (or any other similar DND interface in Lens), would it be possible to disable the ability to drag it and also apply a disabled color and cursor to the drag handle? No point in allowing users to reorder a single item.
src/plugins/chart_expressions/expression_xy/public/components/annotations.scss
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.scss
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.scss
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.scss
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/annotations.scss
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael Marcialis <michael@marcial.is>
…to lens/annotation_fixes # Conflicts: # src/plugins/chart_expressions/expression_xy/public/components/annotations.tsx
Thank you for your review and valuable comments!
That's because it was very short for the queries and with couple of words we couldn't see what the user types. We can try to go back, but I think we have to address this problem. Let me know if you prefer this version to be introduced:
✅ That's not the bug from this PR and it's fixed in another PR (this one: #140313)
✅ I tried to align it, let me know if I understood correctly.
✅ Done!
✅ Fixed it for annotations for now, will keep iterating for other elements tracking it in this issue: #140940 |
…to lens/annotation_fixes
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.
Thanks for making those changes, @mbondyra. This looks great. For your re-review, I've left you a few additional comments below, but nothing worth holding you up over. Assuming you can address them, I'm approving now.
Also, is it possible for us to slightly increase the max-width on the visualization tooltips? If not, no worries (since it's all going to change with the forthcoming tooltip updates). But if it's easy, it might be nice to give some extra space to those additional field key/value rows.
...ck/plugins/lens/public/visualizations/xy/xy_config_panel/annotations_config_panel/index.scss
Outdated
Show resolved
Hide resolved
...blic/visualizations/xy/xy_config_panel/annotations_config_panel/tooltip_annotation_panel.tsx
Outdated
Show resolved
Hide resolved
...blic/visualizations/xy/xy_config_panel/annotations_config_panel/tooltip_annotation_panel.tsx
Outdated
Show resolved
Hide resolved
...blic/visualizations/xy/xy_config_panel/annotations_config_panel/tooltip_annotation_panel.tsx
Outdated
Show resolved
Hide resolved
...ck/plugins/lens/public/visualizations/xy/xy_config_panel/annotations_config_panel/index.scss
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael Marcialis <michael@marcial.is>
Regarding the width of the tooltip, it's unfortunately controlled by charts so cannot be modified from Kibana sourcecode. |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
Tooltip design improvements
These two are done:
But there are some problems with the requirements so I tried to work around them different way.
❓ width of the tooltip is limited by elastic-charts (256px) so I had to move the date below the annotation name instead of showing the two in the same row.
Truncation is a complex problem in this case, and I am afraid we might not able to implement it before Tuesday. For now:
- I added a box shadow to indicate there’s more to show
- I limited annotations to 5 instead of real truncation. Because each annotation can have a lot of extra fields, we cannot easily predict how many we can squeeze. This will sometimes display very small tooltip, but it works in many cases.
Annotation query input
It has been changed to allow the bigger input, let me know what you think!
Field input preserving after changing text decoration
I will work on it on the separate PR - sadly, it's bigger than it seems.
The rest