[Lens] Specify Y axis extent

[flash1293]

Depends on #100108
Fixes #67614
Fixes #99662

As discussed in the issue.

Special cases:

• "Data bounds" is disabled for bar and area series
• Inputs are only shown if "custom" is chosen
• (not as discussed, but I think it makes sense) By default the inputs are empty. This will keep the default of the chart. As it adds another feature (specify only lower bound and keep default upper bound) I thought that's a better default than the current data
• Error is shown if bounds do not include zero for area and bar series

Not ready to review in detail yet.

What do you think @ghudgins @MichaelMarcialis ?

[MichaelMarcialis]

This is looking great! Awesome work, @flash1293. Here's some of my quick thoughts based on your special cases:

"Data bounds" is disabled for bar series

Can we have help text conditionally appear under the button group that explains "Data bounds" is not available for bar charts (since I believe EuiButtonGroups can't have a tooltip)?

(not as discussed, but I think it makes sense) By default the inputs are empty. This will keep the default of the chart. As it adds another feature (specify only lower bound and keep default upper bound) I thought that's a better default than the current data

I'm concerned that it may not be obvious to users that a blank input means "default" value. Is there a common use case for keeping one bound a default value and the other a custom value? If not or an edge case, I'd be more inclined to populate the custom bounds inputs with the actual default values, so they have a point of reference from which to work.

Error is shown if lower bound is attempted to set above 0 for bar series

Rather than relying on the user to fail and see an error message, can we conditionally add help text below the lower bound input that notifies them that the maximum value is zero for bar charts?

[flash1293]

@MichaelMarcialis

Agreed with your points, except for the one about empty inputs, see below

I'm concerned that it may not be obvious to users that a blank input means "default" value. Is there a common use case for keeping one bound a default value and the other a custom value? If not or an edge case, I'd be more inclined to populate the custom bounds inputs with the actual default values, so they have a point of reference from which to work.

Did you test the PR locally? IMHO the experience is pretty natural and it's also how other tools do it (including but not limited to the Visualize editor). I don't have data to judge how common it is to define partial bounds, but it's definitely helpful to just specify the lower bound and keep the upper bound "natural" to cut off an area you know will either never be hit or is a natural point of reference (like "Only show values above 100% because these are the only values relevant to me", sort of filtering them)

[flash1293]

Jenkins, test this

[flash1293]

@MichaelMarcialis Implemented your suggestions except for the prefill with current data thing (see discussion above)

[flash1293]

@elasticmachine merge upstream

[flash1293]

This is a bug fix unrelated to the rest of the PR - the hide endzones parameter was reset when switching between xy chart types

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[MichaelMarcialis]

IMHO the experience is pretty natural and it's also how other tools do it (including but not limited to the Visualize editor). I don't have data to judge how common it is to define partial bounds, but it's definitely helpful to just specify the lower bound and keep the upper bound "natural" to cut off an area you know will either never be hit or is a natural point of reference (like "Only show values above 100% because these are the only values relevant to me", sort of filtering them)

Do we use this "blank equals default" pattern anywhere else within the Lens app? If so, and users at least have some familiarity/experience with such a pattern, I wouldn't be opposed to leaving it as you have it and just monitoring for feedback (or including as part of future usability testing).

If not an existing pattern in Lens though, I'd be a little more concerned. If having the full/default mode for one bound and custom mode for another is a common use case we wish to accommodate, I would recommend an interface akin to the quick example below. It couples the mode selection to individual bounds rather than a single mode selector for both, making it a bit more obvious to the user what is happening.

Thoughts @flash1293 and @ghudgins?

[flash1293]

@MichaelMarcialis check out the screenshots in the original issue description - we do the same thing in the same config popover for the axis title.

I can see the design about different modes for lower and upper bound making sense, but IMHO it's introducing more complexity than necessary.

[flash1293]

@MichaelMarcialis I re-read your comment and I think I there's something I overlooked - the case with axis overwrite is not 100% the same because we show a placeholder. Should we just show the current value as a placeholder in these inputs?

I don't have a strong opinion on this, all of them seem fine to me:

• Blank inputs
• Current values as placeholder
• Prefilled with the current values

Happy to go with your preference for those based on the points raised.

For the last one we could treat the user removing the value as falling back to the default (we do the same for the custom dimension label), so we wouldn't lose the feature of partially specified bounds in any way.

From the technical side blank inputs are a bit simpler to implement, but it's not a big deal.

[MichaelMarcialis]

I can see the design about different modes for lower and upper bound making sense, but IMHO it's introducing more complexity than necessary.

Fair enough. Since you mentioned having used this pattern elsewhere in Lens, I'm OK with keeping it as you have it and just monitoring for feedback.

For the last one we could treat the user removing the value as falling back to the default (we do the same for the custom dimension label), so we wouldn't lose the feature of partially specified bounds in any way.

I think I'm OK with this approach, if I'm understanding correctly. Let me know if this is correct:

1. User selects "Custom" bounds.
2. "Upper bound" and "Lower bound" fields appear, prefilled (i.e. not placeholder) with default/full values.
3. User may customize one or both field values.
4. If user deletes the value from either of these fields, the default/full value is restored on blur.

If this is correct, I think this is fine.

[mbondyra]

@elasticmachine merge upstream

[wylieconlon]

@flash1293 I've tested this PR and found that it wasn't quite what I was expecting based on what I wrote in this comment. I looked through the comment history and it looks like this is probably unintentional, as it wasn't specifically commented on.

• Bar charts must include 0, which means either the lower or upper bound must be >= 0. You're only validating the lower bound, which allows this broken state from -100 to -5:

• Area charts should get the same validation as bar charts, not the same as line charts.
• Edit: this is happening on master too. The right axis settings are appearing every time I have 2 metrics, even if both metrics have the same format
• You're not validating the bounds on line and area charts: I was expecting that we'd validate that lower is less than upper. This part might be hitting a race condition, because most of the time I only see an empty chart:

but sometimes I see a runtime error:

[wylieconlon]

I think these need debouncing, it's causing a lot of rendering churn in the chart and slowing down typing.

[flash1293]

@wylieconlon @MichaelMarcialis I think I addressed all points, could you have another look?

• Prefills custom extents with current data
• Extend validation to area and bars
• Extended validation to zero-inclusive instead of just checking lower bound
• Debounced inputs

Error if lower bound is larger than upper bound (change won't be applied)

Error if zero is not inclusive for area and bar

I couldn't find a good way to show the errors in a single line below both form elements and still keep consistent a11y references, maybe one of you knows a way.

[wylieconlon]

I think you've fully addressed my concerns, LGTM!

[MichaelMarcialis]

Hey, @flash1293. Left a handful of comments and suggestions, but nothing worth holding you up over. Provided you address them, this looks good to me. Approving.

[MichaelMarcialis]

Since we already notify users about the zero inclusion in the custom inputs, I think we can shorten this text by removing that redundant text. Perhaps something like:

Suggested change

	"Bounds of bar and area series always have to include zero and can't be fit to the data",
	"The selected chart cannot be fit to the data bounds.",

[flash1293]

I think we should tell the user which charts can be fit to data bounds, otherwise they just a have a problem, but no obvious solution. I renamed to "Only line charts can be fit to the data bounds"

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: d92429f
• Storybooks Preview

Metrics [docs]

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
lens	150	152	+2

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

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
lens	12	13	+1

Page load bundle

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

id	before	after	diff
lens	34.7KB	34.8KB	+85.0B

Unknown metric groups

API count

id	before	after	diff
lens	161	163	+2

References to deprecated APIs

id	before	after	diff
canvas	29	25	-4
crossClusterReplication	8	6	-2
fleet	22	20	-2
globalSearch	4	2	-2
indexManagement	12	7	-5
infra	261	149	-112
lens	67	45	-22
licensing	18	15	-3
maps	286	208	-78
ml	121	115	-6
monitoring	109	56	-53
stackAlerts	101	95	-6
total			-295

History

• 💚 Build #126225 succeeded 9ea26e9
• 💚 Build #126058 succeeded 0cb01de
• 💔 Build #126048 failed 1860811
• 💚 Build #125027 succeeded bcc6e52
• 💚 Build #124398 succeeded dfc1739

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

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

[sorenlouv]

@flash1293 Thanks for implementing this. Just what I needed for a viz. I ran into some problems when using the custom extend. I'm not sure if the problems were introduced by this PR but I've created a follow-up issue here: #107622