Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat: Re-design probe selection when creating / editing checks #973
Changes from 7 commits
d610556
7e7d72c
f631448
ec9a959
5a575fc
bc523b9
3e9e387
c4a5d42
32e4775
2b3da73
2109637
56b8f34
e5398c6
18c0762
53c4f63
26d9388
7fcec54
cb0db55
0b2609f
cc1413a
9e4c6de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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 theAlert
component withinfo
severity which displays in light blue. Happy to adapt it to the original design if you think that's better though.(edit: changed content text after Chris comments)
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)
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.
In the original design it's suggested to search by more fields, but we don't have all that probes's info in the frontend, so I've limited to what we have available.
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.
Good idea. Addressed in c4a5d42 and 32e4775
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.
Nit: use non-null assertion operator instead of type casting
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.
For another (potential) PR: We should probably not use the same type for a new
Check|Probe
as for an existing one. We have a bunch of components that require an id to be present, having to check for id and/or use non-null assertions in every component isnt super slick.We should perhaps add
type Probe = ExistingProbe | NewProbe
where one is with.id
and the other one isnt? (same for check) 🤷🏻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.
Nit: use non-null assertion operator instead of type casting