[Lens] Normalize Axis title input within popover with compact design

[dej611]

Summary

Fixes #124223

This PR attempts to redesign the Axis title fields within the XY/Heatmap axis popups.

Typing in the text input makes the mode switch automatically to Custom. It is not true the opposite, in fact it is possible to have the Custom mode with an empty title, which will fallback the axis title to the default one. Also the Custom mode will be only temporary in this specific case as closing and reopening the popup Auto will be chosen again.

Here's updated pic of the popover rework after addressing all design feedback:

Previous questions for design

I have few questions, maybe for @MichaelMarcialis here about some styling (solved):

• the gauge popover has a width of 500px to deal with the label box taking 1/3 of the width + the select content that requires some space

  
  • I've applied the same width here, but confined to the axis popups only as it broke the content of the Legend one when I've tried.
    

• Has been considered to normalize also switch inputs? I see two patterns here (and the latter does not look great IMHO):
  

  

Would it make sense to normalize switches as the former one?

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[dej611]

@elasticmachine merge upstream

[MichaelMarcialis]

Thanks for putting this together, @dej611. I'll attempt to answer your questions below:

the gauge popover has a width of 500px to deal with the label box taking 1/3 of the width + the select content that requires some space

* I've applied the same width here, but confined to the axis popups only as it broke the content of the Legend one when I've tried.

Regarding the contents of the legend popover breaking with an increase to the popover width, could you elaborate on what you're seeing? In my quick glance, I didn't see anything that couldn't be remedied by utilizing the full width props provided for the form and button group components, but I may be overlooking something. Let me know.

Has been considered to normalize also switch inputs? I see two patterns here (and the latter does not look great IMHO):

Would it make sense to normalize switches as the former one?

Yes, I agree with you that we should be normalizing the form layouts for our toolbar popovers. In most (if not all) cases, we should be using the display="columnCompressed" prop for all EuiFormRow components in these toolbar popovers. In this particular case, we'd want to use display="columnCompressedSwitch", as the child is a switch. This will put the label on the left and switch on the right, better aligning to the other similarly displayed form rows. Let me know if this is something that should be addressed as a separate issue, and I'll write one up for it. Otherwise, I'll assume we can tackle such changes within this PR.

[dej611]

Regarding the contents of the legend popover breaking with an increase to the popover width, could you elaborate on what you're seeing? In my quick glance, I didn't see anything that couldn't be remedied by utilizing the full width props provided for the form and button group components, but I may be overlooking something. Let me know.

I think there was a problem with the shared VisLabel which had a flexEnd justification that broken the layout with other fields with the 500px width:

I've removed the flexend justification and now it looks aligned with the rest of the rows:

So I think that should be ok now. Let me know if I missed anything.
Note that this change also affected the Gauge popover now which has more room for the title text field:

Yes, I agree with you that we should be normalizing the form layouts for our toolbar popovers. In most (if not all) cases, we should be using the display="columnCompressed" prop for all EuiFormRow components in these toolbar popovers. In this particular case, we'd want to use display="columnCompressedSwitch", as the child is a switch. This will put the label on the left and switch on the right, better aligning to the other similarly displayed form rows. Let me know if this is something that should be addressed as a separate issue, and I'll write one up for it. Otherwise, I'll assume we can tackle such changes within this PR.

I've moved all the switches into a EuiFormRow wrapper now and things looks a bit more aligned:

Given the extra space you may have noticed how the Orientation row now has changed to columnCompressed too as the rest of the popover.
Keeping it the old way made the popover layout not well aligned:

One downside of such change is that the help tooltip when the button group row is disabled shows up now only on buttons hover, and not on label hover:

That is because the tooltip styling broke the row padding behaviour when wrapping the whole row, so I had to move inside to the ButtonGroup field only.

The only exception which is still in rowCompressed is the Bounds min/max layout that is a bit more complex and I thought it makes sense to stay that way. Let me know if that works for you.

[MichaelMarcialis]

I've moved all the switches into a EuiFormRow wrapper now and things looks a bit more aligned:

This is looking good. Would you be willing to try positioning the "Upper bound" and "Lower bound" fields so that they were aligned to the left and right edges below the preceding button group? We could try using an abbreviated prepend label for each field as well (ex. "Upper" and "Lower"). I think it would further tighten the contents of the popover and better convey that the upper/lower bounds are subsets of that button group.

Given the extra space you may have noticed how the Orientation row now has changed to columnCompressed too as the rest of the popover. Keeping it the old way made the popover layout not well aligned:

Yes, having "Orientation" use columnCompressed looks good to keep things aligned nicely.

One downside of such change is that the help tooltip when the button group row is disabled shows up now only on buttons hover, and not on label hover:

I'm OK with this approach. Doubly so because it's my hope that we can change these disabled fields to be hidden in the near future (as the popovers have grown quite large over time). Thanks!

[dej611]

@MichaelMarcialis do you think a dual range input would work as well?

Compared to two number fields it is a more compact design with a clear indication of a range. The only downside of it I see it's the range popover that will show up when editing:

In the PoC above the dual range input has been appended to the Bounds row, which makes a bit hard to deliver the same amount of help/error messages, but that can be mitigated via an independent row on its own.

[dej611]

Also, just noticed about the Data bounds truncation there for the middle button of the group. Any suggestion for a shorter label?

[MichaelMarcialis]

@MichaelMarcialis do you think a dual range input would work as well?

Compared to two number fields it is a more compact design with a clear indication of a range. The only downside of it I see it's the range popover that will show up when editing:

Is there a finite upper bound limit that we impose on the user? If so, I like the idea of using this range input. If there isn't a finite upper bound limit though, I worry about the range input's included slider being honest.

In any case, if we do end up going with the range input, I imagine we can probably do away with the upper/lower prepend/append labels then, as the low and high of the range is already represented via the component.

In the PoC above the dual range input has been appended to the Bounds row, which makes a bit hard to deliver the same amount of help/error messages, but that can be mitigated via an independent row on its own.

True. In the future, I think this will be at least partially addressed by hiding instead of disabling aspects of these forms when they are unavailable. So in that future scenario, we wouldn't need to have the Only line charts can be fit to data bounds message.

For the present however, as we are still disabling instead of hiding, and EUI doesn't support placing a tooltip on individual buttons within a button group, I suggest we either have this help text appear immediately below the bounds button group or as an icon-based tooltip appended to the bounds label. Whichever is easily achievable and doesn't look bad 😄

Also, just noticed about the Data bounds truncation there for the middle button of the group. Any suggestion for a shorter label?

I'm personally fine with shortening this to Data, since bounds is already implied by the field label, but I'll defer to @ghudgins if he has a better name.

[ghudgins]

I'm personally fine with shortening this to Data, since bounds is already implied by the field label, but I'll defer to @ghudgins if he has a better name.

I think Data is problematic because I don't think of this as the data part of the chart at all (it's the axis scale options in d3, as an example). @gvnmagni @elastic/datavis - what should we call the name of the "Data Bounds" control?

• Bounds?
• Data?
• Extent?
• Scale?
• something else...?

[MichaelMarcialis]

I think Data is problematic because I don't think of this as the data part of the chart at all (it's the axis scale options in d3, as an example). @gvnmagni @elastic/datavis - what should we call the name of the "Data Bounds" control?

@ghudgins: Just to clarify, I'm talking about renaming the middle "Data bounds" button in the "Bounds" button group. Not the "Bounds" label for the entire button group, in case that's what you thought I meant here.

[ghudgins]

ah - much better. sorry for missing this earlier. sounds good to me

[dej611]

@MichaelMarcialis reworked the layout for the dual range input without the slider (and proposed to ship it in EUI as separate component):

For line charts the help text between the two fields is not shown:

As you've noticed also the Data bounds button label has been relabelled as Data

[dej611]

@elasticmachine merge upstream

[elasticmachine]

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

[markov00]

@dej611 @MichaelMarcialis @ghudgins
since you are talking about bounds i'd like to propose a simplification and adjustment of the UI to what is currently done by other tools:
we don't necessary need to have a big switch to choose between full extent (including zero), data extent (possible without zero), and custom.
We can show always two numeric inputs: one for the min and one for the max of the range: when you enter a value this will become a configured value, the default (empty) field instead is the min or max of the data.
Adding a small switch we can specify if we want to force the range to include the zero or not. This should be default on and disabled when using bars/areas, but is enabled and can be switched off on lines.

I'm proposing this because:

• the term Full doesn't clearly describe that we are covering the data extent including the zero
• the term Data bounds or Data is not super clear to me

[dej611]

@dej611 @MichaelMarcialis @ghudgins since you are talking about bounds i'd like to propose a simplification and adjustment of the UI to what is currently done by other tools: we don't necessary need to have a big switch to choose between full extent (including zero), data extent (possible without zero), and custom. We can show always two numeric inputs: one for the min and one for the max of the range: when you enter a value this will become a configured value, the default (empty) field instead is the min or max of the data. Adding a small switch we can specify if we want to force the range to include the zero or not. This should be default on and disabled when using bars/areas, but is enabled and can be switched off on lines.

That include zero switch confuses me a little bit.
I can see how it is meant to cover both Full and Data case for the Lines type but looking at the full field row does is not immediate.

The Data option should provide a "compact" scale for the line type, right? Why not use something in that sense instead with/o zero?

[stratoula]

I like it! The settings popover look much better now :) Changed LGTM! I tested it locally and works fine!

[MichaelMarcialis]

Thanks for making those changes, @dej611! This is looking great. I'm leaving a few quick comments/questions below:

• Based on the screenshots you've provided, it looks like range input errors for the upper and lower bounds are replacing the input's help text. Rather than replacing the help text, can we instead show the error in addition to the help text (similar to examples shown in EUI docs)?

• It looks like the Lower prepend and Upper append labels are still present on the range input. Given that the range input already implies that spatially, can we remove those labels to further simplify?

Thanks for your thoughtful comments, @markov00! My initial thoughts are below:

We can show always two numeric inputs: one for the min and one for the max of the range: when you enter a value this will become a configured value, the default (empty) field instead is the min or max of the data.

I'm not a big fan of the pattern of using empty or cleared inputs as a means to revert to using automatic values. While that approach does help to reduce the UI required, I don't think it's very obvious to users. I'd rather the form be slightly bigger (via the button group in this case) and the experience of switching between be more obvious.

Adding a small switch we can specify if we want to force the range to include the zero or not. This should be default on and disabled when using bars/areas, but is enabled and can be switched off on lines.

This is an interesting approach. I like the notion of trying to automatically prevent users from hitting an error for not including zero, but I do fear that the switch may create more confusion in the process. What if we instead took the automation idea and applied it to the upper/lower bound inputs instead? For example, if the user were to change an upper or lower value in a way that the range excluded zero, we simply auto-adjust their value to include zero at a minimum on blur. Thoughts?

Additionally, I think it's worth mentioning how much cleaner this experience will become once we get around to hiding form elements rather than disabling them in these popovers. This will dramatically reduce the visual complexity of this feature and it's popover, as users would initially only see the button group with three options (full/data/custom). When custom is selected, then range input would only appear for further direction from the user.

[dej611]

I think the scope of this PR is getting a bit out of hand.
It started with a redesign of one field and became a full rewrite of all the controls. While I think it is worth discussing the data bounds controls to improve the general UX of the user, but that would better fit into a separate issue.
What do you think @MichaelMarcialis @markov00 ?

[MichaelMarcialis]

I think the scope of this PR is getting a bit out of hand.
It started with a redesign of one field and became a full rewrite of all the controls. While I think it is worth discussing the data bounds controls to improve the general UX of the user, but that would better fit into a separate issue.
What do you think @MichaelMarcialis @markov00 ?

You're right! Apologies for all the comments and ideating in this PR, @dej611. My first two bullet points in my previous comment still stand, but the discussion regarding @markov00's suggestion and my follow-up thoughts on auto-adjusting the upper or lower bound values to include zero automatically on blur can be spun off as a separate issue.

[markov00]

I think the scope of this PR is getting a bit out of hand.
It started with a redesign of one field and became a full rewrite of all the controls. While I think it is worth discussing the data bounds controls to improve the general UX of the user, but that would better fit into a separate issue.
What do you think @MichaelMarcialis @markov00 ?

You're right! Apologies for all the comments and ideating in this PR, @dej611. My first two bullet points in my previous comment still stand, but the discussion regarding @markov00's suggestion and my follow-up thoughts on auto-adjusting the upper or lower bound values to include zero automatically on blur can be spun off as a separate issue.

As stated in my initial comment I'm not widening the scope but it was just a follow-up on the changes proposed by you folks. Happy to discuss it further on a separate issue

[flash1293]

@elasticmachine merge upstream

[dej611]

I've reported @markov00 into a separate issue to discuss layout improvement of the extends control.

[dej611]

@elasticmachine merge upstream

[MichaelMarcialis]

@dej611: Leaving a few small comments below for your review. As these are small issues, I'll approve now so I don't hold you up further after addressing.

• Per my previous comment, it looks like range input errors for upper/lower bounds are replacing the input's help text. Rather than replacing the help text, can we instead show the error in addition to the help text (similar to examples shown in EUI docs)?

• Per my previous comment, it looks like the Lower prepend and Upper append labels are still present on the range input. Given that the range input already implies this spatially, can we remove those labels to further simplify?

[MichaelMarcialis]

Rather than using Overwrite axis title as the placeholder for auto-generated titles, is it possible for us to have this placeholder text reflect whatever the auto-generated title is currently?

[flash1293]

It's possible, but I would like to split this one out as it's not as easy as it seems (information about the currently rendered chart which is required for this is not passed to this component at the moment).

[flash1293]

@elasticmachine merge upstream

[flash1293]

@MichaelMarcialis fixed your previous comments, merging once green.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: fa9cb2a
• Storybooks Preview
• Webpack Bundle Analyzer

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
lens	747	757	+10

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.1MB	1.1MB	+1.1KB

History

• 💔 Build #31312 failed 0ea4b4f
• 💚 Build #30868 succeeded 27275a3
• 💚 Build #30351 succeeded 94d83d6
• 💚 Build #29279 succeeded bc978f7
• 💚 Build #28345 succeeded 263f583
• 💛 Build #27987 was flaky 8d06cda

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