Skip to content

[Lens] small visual flyouts adjustments#124970

Merged
mbondyra merged 19 commits intoelastic:mainfrom
mbondyra:lens/small_visual
Feb 14, 2022
Merged

[Lens] small visual flyouts adjustments#124970
mbondyra merged 19 commits intoelastic:mainfrom
mbondyra:lens/small_visual

Conversation

@mbondyra
Copy link
Copy Markdown
Contributor

@mbondyra mbondyra commented Feb 8, 2022

Summary

Changes:

  1. Fixes [Lens] unnecessary empty space in static value flyout #124966

  2. Add layer design changes:
    before:

Screenshot 2022-02-09 at 12 48 48

after:
Screenshot 2022-02-10 at 22 53 47

  1. Fix 'none' icon:

Screenshot 2022-02-08 at 16 42 13

Screenshot 2022-02-08 at 16 52 36

  1. (not impacting a user)Removing warning about missing keys for errors on color range.

  2. Changes to reference options naming & order

Screenshot 2022-02-09 at 12 46 27

Screenshot 2022-02-09 at 12 46 08

a. Show display name label changed to Text decoration. EuiSwitch converted to EuiButtonGroup.
b. Icon label changed to Icon decoration
c. Decoration position order changed from auto, left, right to left, auto, right
d. Added px to line thickness
e. Order updated just like on the wireframes.

Not implemented

  1. For the Line style (as below), I need the icons svgs:

Screenshot 2022-02-08 at 18 48 43

2. In my opinion calling renaming `decoration position` -> `decoration area` is more confusing, I would confuse it with `fill`... Can we give it a round more?
  1. I feel the same for the name Placement type. How about just category?

Screenshot 2022-02-09 at 14 37 46

  1. Simple headings - good idea, but it's not a simple change with the current architecture so I leave it out for now.

@mbondyra mbondyra added backport:skip This PR does not require backporting Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.2.0 labels Feb 8, 2022
@mbondyra mbondyra marked this pull request as ready for review February 9, 2022 13:43
@mbondyra mbondyra requested review from a team as code owners February 9, 2022 13:43
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@mbondyra mbondyra force-pushed the lens/small_visual branch 2 times, most recently from 5e7c0f6 to bb89b7c Compare February 9, 2022 15:08
Copy link
Copy Markdown
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Great improvement of the UI. 👍

Perhaps the only ambiguity I see (not harmful) is the Decoration position control, which has a different layout compared to other controls with the same values:
Screenshot 2022-02-10 at 12 24 30
Screenshot 2022-02-10 at 12 26 35

Also, it would be nice to have a word wrap that prevent to break words half-way, as the case above.

@MichaelMarcialis
Copy link
Copy Markdown
Contributor

Thanks for putting this together, @mbondyra! I'll begin my review shortly.

Regarding the "not implemented" portion of your PR notes, I'll leave my comments below:

  1. For the Line style (as below), I need the icons svgs:

I'll plan to work up some quick SVG icons and get them added to EUI as soon as I can this week (as I imagine they may have use outside Lens as well). Will let you know when those have been merged in.

  1. In my opinion calling renaming decoration position -> decoration area is more confusing, I would confuse it with fill... Can we give it a round more?

Yeah, I agree with you on this. I changed it to "area" in my wireframes for the sake of space and avoiding a line break at smaller viewport sizes, but it's probably better to have the line break and have the label make more sense than not. I'm fine with keeping it as "position" or changing to another more appropriate name.

  1. I feel the same for the name Placement type. How about just category?

I picked "placement type" here because the user is selecting how the annotation or reference line is to be placed on their visualization. It also seemed to pair well with the subsequent "placement" heading. "Category" sounds a little broad/vague to me, but maybe I'm overthinking it. In any case, I'm not super passionate about the label name. If you or others have a better alternative, I'm not resistant to changing it.

  1. Simple headings - good idea, but it's not a simple change with the current architecture so I leave it out for now.

Understood. Hopefully it's something that can be reconsidered during the annotation development phase, as I do think they help to break-up the forms nicely and give users good visual landmarks.

Copy link
Copy Markdown
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Hey, @mbondyra. Thanks for make some of these preliminary changes that were suggested in the annotation wireframes. These look great overall. I've left a few comments below for your review:

  • For the sake of simplicity, I had suggested changing the current use of the "Display name" label to "Name" (for all dimension configurations) in the annotation wireframes. Is that something we want to change here, or does such a change require more discussion?

  • When both "Text decoration" and "Icon decoration" are set to none, can we hide the "Decoration position" form row, rather than disabling it? I tried to explain my rationale for my modus operandi change in the annotation wireframes, but let me know if you have any questions.

  • Out of curiosity, is it possible to show the icon selected (in addition to the selected icon's name) in the EuiComboBox default state? I didn't see anything that indicated we could do so via the EUI docs, but just checking to see if there's anything I may have missed.

mbondyra and others added 2 commits February 10, 2022 22:12
@elastic elastic deleted a comment from kibana-ci Feb 11, 2022
@mbondyra
Copy link
Copy Markdown
Contributor Author

mbondyra commented Feb 11, 2022

"Display name" label to "Name"

done!

When both "Text decoration" and "Icon decoration" are set to none, can we hide the "Decoration position" form row

done!

Out of curiosity, is it possible to show the icon selected (in addition to the selected icon's name) in the EuiComboBox?

I didn't find a way, but I submitted a feature request in eui: elastic/eui#5628 and @miukimiu helped me out here with adding a prepend prop. I think it works great (see screenshot below)

decoration and axis positions

The design proposed change is to keep 'left, auto, right' and 'top, auto, bottom'. However, for the axis position, at the moment we have 'auto, left, right' and 'auto, bottom, top'. What's the one we should have? My opinion is
left, auto, right and bottom, auto, top because that how the cartesian chart is read (from bottom to top and from left to right). Should we unify it for these cases?

Screenshot 2022-02-11 at 13 38 37

@dej611
Copy link
Copy Markdown
Contributor

dej611 commented Feb 11, 2022

@dej611, the word wrap setting comes from eui form stylings and I think we should keep it this way (also, Chrome handles it much better than Safari you tested on)
Screenshot 2022-02-10 at 22 59 57

Yes, with Chrome it works better. This is not a regression, so no problem with that.

@elastic elastic deleted a comment from kibana-ci Feb 11, 2022
@MichaelMarcialis
Copy link
Copy Markdown
Contributor

Out of curiosity, is it possible to show the icon selected (in addition to the selected icon's name) in the EuiComboBox?

I didn't find a way, but I submitted a feature request in eui: elastic/eui#5628 and @miukimiu helped me out here with adding a prepend prop. I think it works great (see screenshot below)

Yeah, I saw that. I'm good with adopting the pattern that Elizabet mentions there.

decoration and axis positions

The design proposed change is to keep 'left, auto, right' and 'top, auto, bottom'. However, for the axis position, at the moment we have 'auto, left, right' and 'auto, bottom, top'. What's the one we should have? My opinion is left, auto, right and bottom, auto, top because that how the cartesian chart is read (from bottom to top and from left to right). Should we unify it for these cases?

I like your reasoning here regarding the change to bottom/auto/top. Lets use left/auto/right and bottom/auto/top, and make sure we're consistent in our usage across the app.

@mbondyra
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
lens 1.0MB 1.0MB +622.0B

History

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

@mbondyra mbondyra merged commit db7ad4e into elastic:main Feb 14, 2022
@mbondyra mbondyra deleted the lens/small_visual branch February 16, 2022 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Lens] unnecessary empty space in static value flyout

6 participants