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] workspace panel dimensions by chart type #136993

Closed
Tracked by #174147
drewdaemon opened this issue Jul 22, 2022 · 26 comments · Fixed by #168651
Closed
Tracked by #174147

[Lens] workspace panel dimensions by chart type #136993

drewdaemon opened this issue Jul 22, 2022 · 26 comments · Fixed by #168651
Assignees
Labels
Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@drewdaemon
Copy link
Contributor

An idea from #134242 . We could allow visualization types to impose constraints on the dimensions of the workspace panel. These could be specific measurements or just an aspect ratio.

Potential benefits

  • Makes the visualizations in the editor look better "for free"
  • Guides the user toward the best possible visualization appearance
  • Smaller workspace sizes evoke a dashboard panel, reinforcing the connection between Lens and embedding

Example from the metric vis:

Screen Shot 2022-07-22 at 10 24 49 AM

@drewdaemon drewdaemon added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens 8.5 candidate labels Jul 22, 2022
@elasticmachine
Copy link
Contributor

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

@drewdaemon drewdaemon added the Team:DataVis Team label for DataVis Team label Jul 22, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/datavis (Team:DataVis)

@drewdaemon drewdaemon changed the title [Lens] workspace dimensions by vis type [Lens] workspace panel dimensions by vis type Jul 22, 2022
@drewdaemon drewdaemon changed the title [Lens] workspace panel dimensions by vis type [Lens] workspace panel dimensions by chart type Jul 22, 2022
@MichaelMarcialis
Copy link
Contributor

Responded to the thread in Slack, but adding here for the sake of visibility. Personally, I’d prefer to get rid of the white shadowed panel/container for the visualization altogether in Lens, and simply make the center content area (containing toolbar visualization and suggestions) all on a white background. I think it would help reduce visual noise and avoid the box-in-a-box looks that a lot of areas suffer from.

@drewdaemon
Copy link
Contributor Author

Another option suggested by @ThomThomson is to retain the border, but remove the differences in background color between the workspace and the sidebars.

Screen Shot 2022-09-14 at 9 38 03 AM

@markov00
Copy link
Member

@ThomThomson great idea, we love such clean up: this box in box UI pattern start popping up a bit too much around our products, I think is safe and genuine to start cleaning it up a bit more

@stratoula stratoula added Feature:ElasticCharts Issues related to the elastic-charts library and removed 8.5 candidate Team:DataVis Team label for DataVis Team labels Nov 4, 2022
@stratoula stratoula added the impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. label Dec 21, 2022
@stratoula
Copy link
Contributor

Another idea mentioned in our weekly syc, do not change the workspace size but add a shadow instead to distinguish between the visualization and the panel. We should evaluate each one of them

@stratoula
Copy link
Contributor

cc @gvnmagni

@gvnmagni
Copy link

gvnmagni commented May 4, 2023

@MichaelMarcialis I would love to discuss with you about this since we have few implications that are bigger than this single chart

@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis I would love to discuss with you about this since we have few implications that are bigger than this single chart

Yes, I'll put something on the calendar for us to sync up. The biggest question I have (assuming we wish to allow the white workspace panel to shrink/hug certain visualization types) is how will the workspace's DND drop zone be affected? Should the drop zone also shrink? Or should it always maintain the same full width/height dimensions? Something for us to discuss. Stay tuned.

@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented May 12, 2023

Hey, gang. @gvnmagni and I met up to discuss this issue. After discussing possible solutions, we ultimately landed on the same general direction that @drewdaemon described earlier. This includes:

  • By default, visualizations in Lens should be rendered in a panel that occupies the full available width and height of the workspace.
  • For select visualization types (ex. metric, bullet/gauge), we should be able to optionally apply a maximum width and/or a maximum height to the visualization panel.
    • By providing this option, we can also change the metric visualization type from its current gray default background color back to the originally intended white background color.
  • For select visualization types (ex. time series), we should be able to optionally enforce a desired aspect ratio to the visualization panel.
  • In the case that either a maximum width, maximum height, and/or an aspect ratio is applied, the visualization panel will no longer occupy the full available width and height of the workspace. As such, the visualization panel should be centered both vertically and horizontally in the workspace.
  • How this affects the workspace DND drop zone (the green box that appears during a field drag event) will need to be explored further by design. Explorations could include:
    • Allowing the drop zone to match the dimensions of the visualization panel (i.e. drop zone shrinks when the visualization panel shrinks).
    • Always showing a drop zone that occupies the full width/height of the available workspace (regardless of the size of the visualization panel).
    • Visually displaying a drop zone that matches the dimensions of the visualization panel, but actually accepting a drop anywhere in the available workspace.

If we're all in agreement with this direction, let me know and I'll plan to proceed with explorations on the drop zone question when I get a moment.

@stratoula
Copy link
Contributor

@MichaelMarcialis yes this sounds great! Looking forward to the designs :)

@MichaelMarcialis
Copy link
Contributor

@stratoula: As promised, I've explored some possible directions on how to proceed with the workspace drop zones when we're dealing with a Lens visualization type that is restricted to a maximum width, maximum height, and/or an aspect ratio.

Ultimately, I think I much prefer the panel-only direction out of the two explorations shown below. While the full workspace drop zone example does benefit from a consistent and very large size, it doesn't look too great on the metric example and looks like a visual bug on the time series example (with the drop zone appearing flush horizontally, but overflowing vertically). On the other hand, the panel-only direction looks nicer and intentionally placed in all examples. The only downside to this approach is that it will present users with a smaller drop zone for some visualization types, which will ask that they drag fields a bit further from the field list, but that's a tradeoff I'm willing to accept for the time being (and one we can monitor for feedback).

CCing @gvnmagni in case he has any additional thoughts.

Panel-Only Drop Zone (My Preferred Direction)

Metric Example

Panel Only

Time Series Example

Panel Only

Full Workspace Drop Zone

Metric Example

Full Workspace

Time Series Example

Full Workspace

@gvnmagni
Copy link

I love how clean and elegant it looks in the first option, I would prefer that as well. We can eventually enlarge that if users have troubles with it, but I doubt that it would be a problem 😊

@stratoula
Copy link
Contributor

@MichaelMarcialis thanks so much! The panel drop zone was my personal preference so happy that we agree!

@drewdaemon drewdaemon self-assigned this Oct 11, 2023
@drewdaemon drewdaemon removed the Feature:ElasticCharts Issues related to the elastic-charts library label Oct 11, 2023
@drewdaemon
Copy link
Contributor Author

drewdaemon commented Oct 19, 2023

A few learnings:

Aspect ratios are easy(-ish)

Defining an aspect ratio without worrying about the actual dimensions is straightforward (e.g. line charts should be 16:9). Can use flexbox with the CSS aspect-ratio property.

Predicting metric dimensions

Metric dimensions are supposed to follow this logic

  • Is small multiples?
    • No — width = 300px, height = 300px
    • Yes — width = maxColumns * 200px, height = Math.ceil(numMetrics / maxColumns) * 200px

In the small multiples case, numMetrics can sometimes be known before receiving data. In other cases, we can't know until we receive data.

Breakdown function can know tile count before data
Top values Yes
Filters Yes
Intervals No
Date histogram No

So, width can always be known based on configuration, but height sometimes can't be known until we have data.

Timing WRT to chart render

The workspace panel should be resized in the same render cycle as the chart is drawn.

Resizing too soon

Chart render is always after data is received. So, if the workspace is resized as soon as we know what size it should be (sometimes before data, and always before chart render) the previous chart will be displayed distorted before the new chart is rendered.

Screenshot 2023-10-19 at 9 04 21 AM

Panel is resized before chart updates.

Screenshot 2023-10-19 at 9 04 26 AM

New chart is eventually rendered into the panel.

Resizing too late

However, if we wait to set the dimensions until the charts library notifies of a completed render, the chart will be drawn in an unsized dimension panel which is then sized, thereby triggering a second chart render.

Screen.Recording.2023-10-19.at.9.22.19.AM.mov

Conclusion

The existing charts notifications need to be extended with a "will-render" event that notifies the consumer that the chart will be rendered on the next cycle. That way, we can cue up the dimension change to coincide with the chart's render.

@nickofthyme does this sound reasonable?

Where dimensions should be computed

There seem to be two viable options as to where we compute the dimensions

  1. the visualization class
  2. the expression renderer

With 1, we get the flexibility of knowing the panel dimensions before data has been received in some cases. I don't foresee much use for this but I guess it could be useful for #136995 if the user was to create a Lens visualization from dashboard and then click Save and return before the render is complete. But it feels like an unlikely case perhaps?

With 2, we don't need to move as much code and it makes sense to me that the expression renderer would request optimal dimensions/aspect ratio depending on the information it has.

So, I lean toward 2, but I'm looking for feedback (cc @stratoula )

@nickofthyme
Copy link
Contributor

Great analysis here!

However, if we wait to set the dimensions until the charts library notifies of a completed render, the chart will be drawn in an unsized dimension panel which is then sized, thereby triggering a second chart render.

Is this second render ever noticeable by eye?

The existing charts notifications need to be extended with a "will-render" event that notifies the consumer that the chart will be rendered on the next cycle. That way, we can cue up the dimension change to coincide with the chart's render.

Yeah I think this sounds like a possible solution and I think pretty easy to implement from the charts size, just need to find where/when to fire the event to have the right affect.

@drewdaemon
Copy link
Contributor Author

However, if we wait to set the dimensions until the charts library notifies of a completed render, the chart will be drawn in an unsized dimension panel which is then sized, thereby triggering a second chart render.

Is this second render ever noticeable by eye?

Good question. At least on my computer, it looks more like a flurry of activity. I don't think I'd notice the distinct stages if I didn't know what to look for. If you slow the video above down to .5 speed, you can see the stages.

Screenshot 2023-10-19 at 10 17 49 AM

Full workspace render

Screenshot 2023-10-19 at 10 18 02 AM

After resize but before re-render

Screenshot 2023-10-19 at 10 18 11 AM

After resize and re-render

But, when I throttle the CPU to simulate a slower machine, it becomes even more noticeable.

@nickofthyme
Copy link
Contributor

Yeah that's not good. Also thanks for sharing the playback speed option, didn't know that existed! 🎉

@stratoula
Copy link
Contributor

Yes this is not good, even without slowing the video I can see the flickering so I like the idea of the consumer notification.

With 2, we don't need to move as much code and it makes sense to me that the expression renderer would request optimal dimensions/aspect ratio depending on the information it has.

I agree with option 2 Drew, it makes more sense to be on the expression renderer side. This is what I would expect as a developer 👍

@dej611
Copy link
Contributor

dej611 commented Oct 23, 2023

Breakdown function can know tile count before data
Top values Yes
Filters Yes
Intervals No
Date histogram No

For Intervals you can find the number of buckets in two ways:

@drewdaemon
Copy link
Contributor Author

drewdaemon commented Oct 23, 2023

Another twist. Even after the willRender hook is in charts, we have a problem.

  1. charts library plans chart dimensions based on current container size
  2. charts library schedules a render and notifies consumer
  3. consumer schedules a dimensions update
  4. browser updates container dimensions and draws chart simultaneously
  5. charts library detects the discrepancy
  6. charts library redraws chart

Or visually:

Screenshot 2023-10-23 at 2 14 50 PM

charts library uses existing container dimensions to plan render

Screenshot 2023-10-23 at 2 15 01 PM

render and container dimension update occur simultaneously

Screenshot 2023-10-23 at 2 15 10 PM

charts library resizes chart to new dimensions

@drewdaemon
Copy link
Contributor Author

Blocked on elastic/elastic-charts#2221

@drewdaemon
Copy link
Contributor Author

Blocked on elastic/elastic-charts#2238

@drewdaemon drewdaemon added blocked and removed blocked labels Nov 9, 2023
@drewdaemon
Copy link
Contributor Author

Decided synchronously not to block this effort on elastic/elastic-charts#2238

@ninoslavmiskovic
Copy link
Contributor

Is the status on this still blocked ? @drewdaemon + @stratoula

@stratoula
Copy link
Contributor

We are just waiting for this #170914 to be merged

drewdaemon added a commit that referenced this issue Jan 14, 2024
## Summary

Close #136993

All charts remain the same except the following

## Metric
Each tile gets `200px` if there are multiple and `300px` if it's a
single. The default background is EUI empty.

<img width="600" alt="Screenshot 2023-10-30 at 3 55 33 PM"
src="https://github.com/elastic/kibana/assets/315764/74d7e6dc-c90a-4f60-bf56-94ab1556ad42">

<img width="600" alt="Screenshot 2023-10-30 at 3 56 36 PM"
src="https://github.com/elastic/kibana/assets/315764/3254160a-b18a-4c04-b059-f20b8f1f246a">

## XY
Vertical time-series charts have `16:9` ratio but will stretch to any
available width and won't shrink below 300px height.


https://github.com/elastic/kibana/assets/315764/3e056ae8-bc65-4851-9ad9-6c8a81bdf58a




## Gauge

Gauge gets `600px` for the long side, `300px` for the short side.

<img width="600" alt="Screenshot 2023-11-28 at 11 22 20 AM"
src="https://github.com/elastic/kibana/assets/315764/2b774515-f060-4c26-a0aa-efdeebfff0e5">

<img width="600" alt="Screenshot 2023-11-28 at 11 22 33 AM"
src="https://github.com/elastic/kibana/assets/315764/62181021-d88a-4cb6-862b-42768a2df13e">



## Known issues
- [ ] text resizing on the metric
elastic/elastic-charts#2238


https://github.com/elastic/kibana/assets/315764/0162f461-b544-44a9-971c-b2a0265d7d3a

- [x] gauge isn't showing veil on willRender

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

Flaky test runner:
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4755

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: nickofthyme <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants