grafana / scenes

Build Grafana dashboards directly in your Grafana app plugins.
https://grafana.com/developers/scenes
Apache License 2.0
133 stars 20 forks source link

AdHocFiltersVariable - new ad hoc filters UI #830

Closed Sergej-Vlasov closed 4 weeks ago

Sergej-Vlasov commented 2 months ago

This is a POC work on the new UI (combobox style) for the AdHoc Filters.

package.json additions:

To try locally: follow the steps in the Demo App README, enable Scenes Test App in plugins and navigate to http://localhost:3000/a/grafana-scenes-app/demos/adhoc-filters/new-filters

The designs for this can be found in this figma link

📦 Published PR as canary version: 5.11.1--canary.830.10631142943.0
:sparkles: Test out this PR locally via: ```bash npm install @grafana/scenes-react@5.11.1--canary.830.10631142943.0 npm install @grafana/scenes@5.11.1--canary.830.10631142943.0 # or yarn add @grafana/scenes-react@5.11.1--canary.830.10631142943.0 yarn add @grafana/scenes@5.11.1--canary.830.10631142943.0 ```
dprokop commented 2 months ago

Wanted to try it out and was confused it doesn't work... Because my data source was non existent, the underlying error has not been shown. Then i have configured my ds and now it returns dimensions and values, but... there's no indication an async action is taking place.

@Ijin08 and @catherineymgui - we need some designs for these two scenarios, seems like those are not included in the Figma file.


@Sergej-Vlasov - i've tried this out and it works surprisingly smooth as for the first POC. Things I think will make sense

  1. Backwards key should remove the preceding filter - now it feels an awkward key combo to Shift+tab to get to the preceding filter.

  2. Flaky edit behavior - after Shift+tab and pressing enter, and then pressing Tab again creates filter in a wip-ish state:

    image
  3. X button on filter is inconsistent. When clicked, it removes the entire filter, but when Shift+Tab and Enter, it removes the filter value only (see 2 above as well).

  4. In multi-filter scenario, I need to be precise to activate the next filter creation. I need to click precisely on the input placegholder, otherwise the filters input will not get focus. I think clicking anywhere in inside the filters combobox should focus the underlying input. Of course apart from clicking on the individual filter pill.

    image
  5. Filter that's about to be edited should preserve it's position, othewise the experience is confusing. See the video, when I start editing the filter, the label goes back to the previous line:

https://github.com/user-attachments/assets/568473a9-1443-4176-9fdf-744a48c2d56a

  1. Thing to consider (cc @Ijin08 & @catherineymgui) - should the individual filter pill become editable after a single or double click? My personal take is it should be "forced" to be edited by double clicking on it.
Sergej-Vlasov commented 2 months ago

@catherineymgui @Ijin08 as we discussed the possible error cases to consider:

  1. API error - something wrong with api , data or any other X factor such as network error
    • solution could be the red/yellow warning icon at the end of input with tooltip
  2. Options fetch error - could not retrieve the options for the dropdown (this can be network/api related)
    • solution could probably be a dropdown option that says "Failed to fetch. Try again"
catherineymgui commented 2 months ago

@Sergej-Vlasov I think your solutions for errors make sense. Let me know if I need to mock up anything.

@dprokop I agree with most of your points except editing on clicks. I was thinking of editing on one click (because what happens otherwise, if someone clicks once)? What do you mean by ‘forcing’ editing?

Additional thoughts / feedback. Would love discussion:

https://github.com/user-attachments/assets/faf8e281-8e46-4b7c-a906-c9dde915610e

Mocked up example of what I mean for hover and edit state:

https://github.com/user-attachments/assets/7442ea1b-ad9c-4641-889d-b139d7cc56bd

https://github.com/user-attachments/assets/0f3fae0d-e906-47bd-83e9-8b65e36404c1

Screenshot 2024-07-16 at 17 32 38

General question, what will be part of this POC? Here are some additional functionality we talked about but not sure if this is after POC:

Sergej-Vlasov commented 2 months ago

@dprokop @catherineymgui @Ijin08 pushed latest improvements. Here is what has been done:

@dprokop - addressed all of your feedback except for:

@catherineymgui - addressed the following:

@catherineymgui we will discuss the following points during sync:

@dprokop would be great to discuss where POC scope ends during our sync to have clarity on the @catherineymgui comment:

General question, what will be part of this POC? Here are some additional functionality we talked about but not sure if this is after POC:

  • Typing to filter / select value
  • Truncated pills if it is too long (we talked about it briefly in our meeting, but I know logs is asking for this because they may have very long filters)
  • Custom values
  • Collapse / expand rows

Current TO-DO list:

adamquan commented 1 month ago

This is going to be very helpful for folks at Palantir! They are very excited about this. Thank you, team!

dprokop commented 1 month ago

UI/UX feedback (mainly for @Ijin08 and @catherineymgui)

  1. Loading indicator - i don't think the proposed solution works well, it's barely visible and static, gives an impression like nothing is going on actually:
image

I think the current filter's loading indicator (spinner) works a bit better, and is aligned with design system(https://developers.grafana.com/ui/latest/index.html?path=/story/forms-input--simple&args=loading:!true)

image
  1. Error indicator Similar comment as with loading indicator, i don't think the proposed solution works well:
image

It's hard to read and is not in line with the design system: https://developers.grafana.com/ui/latest/index.html?path=/story/forms-input--with-field-validation&args=loading:!true

  1. Same comment that @torkelo made, the dropdown cuts off values so it's very hard to see values longer than a few chars

image

Feature patiry / functionality:

  1. Filteing dimensions and values: It filters by exact match, opposite to the current ad hoc filters that are more fuzzy.
catherineymgui commented 1 month ago

I agree with most of @dprokop's points except:

Error on dropdown - is this a fetch error that only shows when you go into the dropdown? I think it's okay to keep in it, as it's the same pattern as when there are no options found in Explore (for whatever reason).

Screenshot 2024-08-20 at 10 23 58

I would just update the text? "There is an error fetching the labels. Try again."

As a sidenote - how can I test these loading and error states?

dprokop commented 1 month ago

I would just update the text? "There is an error fetching the labels. Try again."

How does this align with the design system @catherineymgui? Also Try again is not very intuitive here as the user needs to blur and focus the input in order to "try again".

catherineymgui commented 1 month ago

@dprokop I don't think we have any comprehensive error handling guidelines in the design system? :cry:

I know what you showed is more for missing user input (like in a form - oh please fill this out, we are missing some additional content or you need to correct this).

https://grafana.com/developers/saga/components/input/#states

Re: Text

I am struggling to think of a way to tell user what to do that isn't super awkward... how about this "There is an error fetching labels. Click into the input field again to retry." ??

Sergej-Vlasov commented 1 month ago

What has been done and what is remaining for later

New filters UI - design

Sergej-Vlasov commented 1 month ago

@ashharrison90 addressed all points except for operator filtering. uFuzzy setup needs adjustment, but cannot quickly figure out how. Will pick it up first thing tomorrow morning 👍

Edit: found a way to filter operators, changes pushed

Sergej-Vlasov commented 1 month ago

@bfmatei addressed all comments 👍

I would rather fix responsive layout in a separate PR after this, because it requires adjustments to VariableValueSelectWrapper and that will need extra testing. So in order not to drag this PR out I will address that separately

grafanabot commented 4 weeks ago

:rocket: PR was released in v5.11.1 :rocket: