opensearch-project / OpenSearch-Dashboards

📊 Open source visualization dashboards for OpenSearch.
https://opensearch.org/docs/latest/dashboards/index/
Apache License 2.0
1.67k stars 875 forks source link

[BUG] Metric Visualization text value is not fully displayed/partly truncated (CSS line height issue) #1643

Closed ghost closed 2 years ago

ghost commented 2 years ago

Describe the bug

For metric visualizations, the displayed text is not displayed/truncated at the bottom. It only affects letters that protrude downwards like @, p, q, j, ...

To Reproduce Steps to reproduce the behavior:

  1. Go to 'Add new visualization'
  2. Click on 'metric'
  3. Create a new metric visualization and press save
  4. Gather data (incls. values with letters that protrude downwards like @, p , q, j...)
  5. See text value in metric visualization
  6. See error

Expected behavior Letters of values in metric visualizations should not be truncated and be fully displayed.

OpenSearch Version Official Docker-image: opensearchproject/opensearch:1.3.2

Dashboards Version Official Docker-image: opensearchproject/opensearch-dashboards:1.3.2

Plugins

Please list all plugins currently enabled.

Screenshots

02_metric_vis_text_css_bug 01_metric_vis_text_css_bug

Host/Environment (please complete the following information):

Additional context

Add any other context about the problem here

From my point of view the problem exists due to a missing CSS line height setting in CSS class '.mtrVis__value' It could be fixed by adapting the following CSS settings:

From current CSS values: .mtrVis__value { max-width: 100%; overflow: hidden !important; text-overflow: ellipsis !important; white-space: nowrap !important; word-wrap: normal !important; font-weight: 700; }

To new fixed CSS values: .mtrVis__value { max-width: 100%; overflow: hidden !important; text-overflow: ellipsis !important; white-space: nowrap !important; word-wrap: normal !important; font-weight: 700; line-height: normal; # This is the fixing line }

I hope I was able to support you at least a little and would like to thank you again for your commitment and the great product!

All the best Patrick

kavilla commented 2 years ago

Hello @netimate,

I see you have the propose changes, did you want to raise a PR with these changes?

Will assign @joshuarrrr to follow-up if you prefer we take it over then let us know!

[Triage] We should follow-up if this is the correct class to target. And then let UX team know what the CSS values are because they are working on global spacing via tagging them on PR. cc: @kgcreative @opensearch-project/opensearch-ux

joshuarrrr commented 2 years ago

I took a little time to look at the existing implementation of the metric visualization. While there are likely a number of other ways we could improve the rendering component itself, I think the proposed solution of setting line-height: normal; on either .mtrVis__value (or, alternatively, on .mtrVis__container) is the best short-term fix.

Findings

The metric visualization component has some quirks in the way it's currently implemented.

  1. The visualization options panel allow the user to specify any font size for the metric value between 12 and 120 pts (why pts is chosen as the fixed unit is not explained).
  2. The implementation doesn't use any EUI components, it just passes the user-configured options as inline styles: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/665424e563521a6556ded5d6e3ea4090956a76c0/src/plugins/vis_type_metric/public/components/metric_vis_value.tsx#L62-L65
  3. It also uses formatters to produce and inject raw HTML: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/665424e563521a6556ded5d6e3ea4090956a76c0/src/plugins/vis_type_metric/public/components/metric_vis_value.tsx#L86-L94
  4. It appears the the metric value is intended to truncate long lines, but for that to work, we'd need to set a max-width on mtrVis__container (or otherwise set the parent width). Note that, even though we never effectively truncate, the inclusion of the truncation styles make the reported bug worse, by preventing wrapping and hiding overflow (what actually accounts for the descenders appearing clipped): https://github.com/opensearch-project/OpenSearch-Dashboards/blob/6d1675cd989cdb8c271ee0d89227166162658b17/src/plugins/vis_type_metric/public/components/metric_vis.scss#L16-L17

Because of 2. (and 4), absent any other styling, the line-height value falls back to the body reset value of 1, with all overflow hidden.

From the look of it, 3. also seems like a design decision we may want to revisit. The HTML formatter logic is fairly complex, and even allows for multiline output (if the values contain multiline text). If we want to continue to use this approach, the metric visualization should be able to nicely display anything the formatter may spit out. Edge cases include concatenation aggregates, such as this absurd configuration (shown with truncation disabled for illustration purposes):

Screen Shot 2022-05-31 at 4 38 33 PM
ghost commented 2 years ago

Hello @kavilla @joshuarrrr ,

thanks in advance for your quick and competent action!

I would kindly ask you to take over the further steps in this case.

For future requests I will be happy to set up the project locally in order to be able to request PR directly.

Thank you for your support and time it is a pleasure to work with you

All the best Patrick

joshuarrrr commented 2 years ago

Got it, thanks @netimate, I'll go ahead and make the PR.