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

Prometheus: Ensure values in metric selector are visible #87150

Conversation

NWRichmond
Copy link
Contributor

@NWRichmond NWRichmond commented Apr 30, 2024

What is this fix and why do we need it?

In the Prometheus data source's Query Builder, users must select a metric name. As described in #85829, the select can display options below the viewport. This happens because we are lazy-loading the options, and the component determines the placement (top/bottom) of options (based on available space above/below the select) before he options have finished loading. This presents a UX issue because users can get the false impression that metric names are missing from the select.

In addition to some light refactoring, this PR changes the select in the following ways:

  • The select can correctly place options above when there isn't enough space to place them below.
  • The select leverages the <CustomScrollbar />'s showScrollIndicators prop to indicate to the user when there are results above or below their current scroll position.

Before

(on main)

Note the various issues described above and in #85829.

demo.metrics.select.issues.in.before.state.mov

After

(on 85829/fix-prometheus-query-builder-metric-selector-display-beyond-viewport)

In the beginning of the video, note how the top and bottom of the options are greyed-out when the top-most and bottom-most options are out-of-view. This provides a visual cue to the user that they must scroll to reach the bottom or top. Next, note how regardless of how much space is available to render the select, the options are always visible.

demo.metrics.select.scroll.indicators.and.options.placement.without.border.changes.mov

Who is this feature for?

Prometheus data source users who rely on the Query Builder.

Which issue(s) does this PR fix?

This PR fixes #85829.

Special notes for your reviewer

N/A

Future work

These changes could also be applied elsewhere. We should examine any <Select /> or <AsyncSelect /> that uses onOpenMenu to lazy-load options, as they likely suffer from the same UX issues. In the Prometheus data source, LabelFilterItem.tsx and LabelParamEditor.tsx in packages/grafana-prometheus/src/querybuilder/components are candidates.

Please check that:

  • It works as expected from a user's perspective.
  • (Not applicable) If this is a pre-GA feature, it is behind a feature toggle.
  • (Not applicable) The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@NWRichmond NWRichmond requested a review from a team as a code owner April 30, 2024 18:17
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone Apr 30, 2024
@NWRichmond NWRichmond changed the title Prometheus: fix query builder metric selector display beyond viewport Prometheus: Ensure selectable values in metric selector are visible Apr 30, 2024
@NWRichmond NWRichmond changed the title Prometheus: Ensure selectable values in metric selector are visible Prometheus: Ensure values in metric selector are visible Apr 30, 2024
Copy link
Contributor

@itsmylife itsmylife left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@NWRichmond NWRichmond merged commit 21afe3d into main May 2, 2024
20 checks passed
@NWRichmond NWRichmond deleted the 85829/fix-prometheus-query-builder-metric-selector-display-beyond-viewport branch May 2, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus: Query builder's Metric select is placing options below the viewport
3 participants