Skip to content
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] Ability to add some top margin to the chart #171097

Open
dej611 opened this issue Nov 13, 2023 · 8 comments
Open

[Lens] Ability to add some top margin to the chart #171097

dej611 opened this issue Nov 13, 2023 · 8 comments
Labels
blocked enhancement New value added to drive a business result Feature:Lens impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@dej611
Copy link
Contributor

dej611 commented Nov 13, 2023

Describe the feature:

Sometimes it would be useful to have some padding on the top chart.
Few use cases:

  • a reference line above current data
  • keep always some empty padding on top of existing chart

Describe a specific use case for the feature:

A typical reference line example:

Screenshot 2023-11-13 at 14 50 38

In this case Lens makes its best to take into account the reference line on top of the chart (auto-magically setting a Y custom bounds) but it does not take into account any padding.
A hacky workaround for this would be to set a transparent reference line (either statical or dynamically computed) to push higher the Y axis top:

Screenshot 2023-11-13 at 14 56 54

but ideally it would be nice to be able to express some padding at chart level in pixels making it indipendent from current unit used.

cc @nickofthyme WDYT?

@dej611 dej611 added blocked enhancement New value added to drive a business result Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Nov 13, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@stratoula
Copy link
Contributor

I feel that the Round to nice values is doing a good job on the majority of the charts. There are though cases, as the reference lines one that Marco mentions, which would need some love.

My personal opinion is to add this padding (not sure if EC allow it) programmatically in specific cases such as reference lines case mentioned above and not give this ability to the users.

@nickofthyme
Copy link
Contributor

Couple thoughts...

First, to answer...

My personal opinion is to add this padding (not sure if EC allow it) programmatically in specific cases such as reference lines case mentioned above and not give this ability to the users.

Yes this should be doable but this is simply an option on the Axis.domain prop, that just expects a numerical value. No chart context is provided to determine the padding to be used.


Second, about the reference line case in particular, we have an option for the LineAnnotation to render a markerBody that adapts to fit inside the chart near the ends, this behavior is not applied to the marker. But it can collide with others (elastic/elastic-charts#1110) so may not be the best approach but just thought I'd mention it. See this story for demo.

Screen Recording 2023-11-13 at 10 03 21 AM


Third, It would be nice for it to be automagic where the user doesn't have to think about it. In Aggs-based we do allow for a Domain margin but only when scaling to the data bounds, which is not an ech mandate. You can play around with the padding options in charts here.

Visualize Library - Elastic 2023-11-13 at 10 10 11 AM

I would be happy with always having all these options:

  • fit to the data domain
  • nicing the domain
  • provide a domain padding - currently we only allow a single value to apply to the upper and lower ends of the domain but we have an option that defaults to constrain the domain from crossing 0 unless the data goes negative.

All of these are independent options and not mutually exclusive from an ech perspective. The only case these options would be disabled is if a custom domain is used.

On that note, we also don't allow setting only one side of the domain which I think could be nice, but I'm not sure how the options would/should be applied if only the upper or lower bound is defined.

@stratoula stratoula added impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. and removed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Nov 14, 2023
@stratoula
Copy link
Contributor

stratoula commented Nov 14, 2023

Thanx Nick! I am changing this to needs-assessment to discuss it on our grooming session and take a decision. (I still think is impact low)

@dej611
Copy link
Contributor Author

dej611 commented Nov 14, 2023

Nice, there are both pixel and data options for the padding, which I think make sense.
Also, it would be nice to remove some Lens code that handles the domain fit for the reference lines in case these are out of data domain (higher or lower).

On that note, we also don't allow setting only one side of the domain which I think could be nice, but I'm not sure how the options would/should be applied if only the upper or lower bound is defined.

This would be perfect. Probably we need to experiment a bit with the current feature and assess if it makes sense to do it or rather wait for this new setting.

@nickofthyme
Copy link
Contributor

Nice, there are both pixel and data options for the padding, which I think make sense.

Glad someone appreciates my added optionality 😄

Also, it would be nice to remove some Lens code that handles the domain fit for the reference lines

Agreed!

@timductive timductive added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Nov 15, 2023
@nickofthyme
Copy link
Contributor

nickofthyme commented Nov 15, 2023

FYI the padding is applied BEFORE nicing, so not as you expected @dej611. So a 5px padding with nicing could push the domain much larger than 5px.

Screen Recording 2023-11-15 at 09 20 15 AM

The first 2 changes toggles between 0px and 5px padding with yNice set to true, notice the top domain jumps from 200 to 300. The last 2 changes again toggles between 0px and 5px padding with yNice set to false and only applies the desired padding to the top of the domain.

I think we can add this control in charts to apply the padding before or after the domain niceing or take the max of the two.

@stratoula
Copy link
Contributor

So we have 3 options here:

  • Always add a 5 (or bigger)px padding in Lens charts. We are not sure how this is going to look though and if this is what the solutions teams want
  • Add the padding to the overwrites, so the consumers can adjust with the value they prefer. It is not displayed in the editor though.
  • Do what Nick proposes here [Lens] Ability to add some top margin to the chart #171097 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked enhancement New value added to drive a business result Feature:Lens impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

5 participants