-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feat: Re-design probe selection when creating / editing checks #973
Conversation
37eff0a
to
ec9a959
Compare
return ( | ||
<Alert | ||
title="You haven't set up any private probes yet." | ||
severity="info" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original design used a different color and icon for this message, but with the intention of reusing and not modifying the elements we get from grafana/ui
for consistency with other parts of the app, I've used the Alert
component with info
severity which displays in light blue. Happy to adapt it to the original design if you think that's better though.
const searchValue = event.target.value.toLowerCase(); | ||
|
||
const filteredProbes = probes.filter( | ||
(probe) => probe.region.toLowerCase().includes(searchValue) || probe.name.toLowerCase().includes(searchValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, on that note, let's create our own internal mapping of it and make a tech-debt ticket that we would rather the api provides us this info at some point in the future. Our probes are static enough (despite the current migrations...) that I feel comfortable we can hard-code this in our repo as it gives a huge UX boost to the user.
As a user I'd just be confused why the docs website provides this level of info but the app doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 🔥 🔥 Awesome job so far. Few comments I've left inline.
Two slightly more general comments.
There actually is a third (!) state that a checkbox can have and we do utilise at Grafana: indeterminate so we could add that in situations where not all the locations are used in a region.
We'll have to check this one with @majavojnoska but I wasn't sure if the brackets beside the region header were supposed to be showing how many regions are available or how many you have selected currently?
So in the following example it would be: Private Probes (1) - AMER (1) - EMEA (0)
Probes are automated tools that test websites and apps. They act like users by sending requests and checking the | ||
responses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update the copy here as it isn't too specific to private probes but probes on a whole. @heitortsergent any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to the following, based on the text from the docs (https://grafana.com/docs/grafana-cloud/testing/synthetic-monitoring/set-up/set-up-private-probes/#set-up-private-probes)
Also, add indeterminate state to checkbox
I believe I addressed all your comments @ckbedwell 🕺 Let me know otherwise, thanks! |
At first, I thought it would be good to show how many probes are available. But now that you've asked, I think it makes more sense to show how many probes you've actually chosen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't care so much about the comments. Overall I love it! 🤩
The checkbox and probe identifier are not aligned
Have we considered including private probes in their respective region (including the fact that a private prove can have no region) (there is no indication of what region a private probe belongs to)?
It's a bit
src/hooks/useLocalStorage.ts
Outdated
@@ -0,0 +1,20 @@ | |||
import { useEffect, useState } from 'react'; | |||
|
|||
export const useLocalStorage = <T>(key: string, initialValue: T): [T, (value: T) => void] => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super keen on this hook.
-
This hook will always set a default value. Even if we change the default value at some point, the user will still be stuck on the old default (without making an active choice).
-
The hook has no context/listeners to sync value change between components.
I'd rather see us using usehooks-ts
.
Note: even if we'd use usehooks-ts
, I'd suggest to use the option initializeWithValue: false
I could live with this hook existing if:
- keys were namespace (eg
grafana.syntheticMonitoring[<instanceId>].<key>
) - There were comments (pref JSDoc) clearly stating that state is not synced
initialValue
changed name todefaultValue
, with an option{setDefaultValue: Boolean = false}
(which would return the default value without writing it to localStorage).- (feature request 😅) if the hook would would allow the developer to select
local|session
storage with an option/param eg.useClientStorage<T>(key: string, defaultValue?: T, options?: ClientStorageOptions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added usehooks-ts
instead, love the set of hooks it provides!
Thanks for the review @w1kman! Really good suggestions 🎖️
Regarding this, we could do that as the API provides us with the region. But I also think it's good to be able to differentiate them from all other public locations in a separate column as a private probe for easy access. We don't have the provider or the country name as shown in the designs (https://www.figma.com/design/WXbmUSpCMlJHG8fpjvmGT1/%F0%9F%8F%97%EF%B8%8F-Synthetics-%5BDEV-READY%5D?node-id=2244-15776&node-type=frame&m=dev) but I wanted to respect the intention of showing them separately from all other probes. What do you think about Thomas idea, @majavojnoska? |
I need to re-review the code but wanted to leave a quick comment with two things I noticed UX-wise: Can we add some sort of clear or x button when the search input has a value? This is a screenshot below when I've just put a space, so it's hidden a load of the probes but it isn't clear there are more probes and/or why they are hidden: And on a similar note, if you search for something and no probes match, can we have a zero state mentioning no probes match that criteria or some such? |
- add button to clear search - add message when no results
Improved the filter input by adding a Clear button when it has value and a message when there are no results: Screen.Recording.2024-11-08.at.12.06.01.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👏 🔥 🌶️
I'm unsure if Thomas's proposal is to categorize public probes by region or to mix public and private probes within the same regional groups. I was hesitant to add regions to private probes because it seemed overly granular and because of: User flexibility: As Chris explained to me, users can input any coordinates they desire, which may not align with specific regions. Usage patterns: The data shows that users primarily create a small number of private probes (average of 1.66 per user). Assigning regions to these could clutter the UI and potentially hide the private probes. Promotional opportunity: A dedicated section for private probes can serve as a valuable promotional space for this feature. Some usage stats for private probes:
Usage stats for public probes: 98.32%
|
Closes #822
Implements option 2 from #822 (comment) where we display the probe locations in checkbox lists separated in columns according to whether they're private or the region they belong to.