-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Synthetics UI] Update copy for test now functionality on private locations #137912
[Synthetics UI] Update copy for test now functionality on private locations #137912
Conversation
Pinging @elastic/uptime (Team:uptime) |
@@ -82,7 +82,8 @@ export const TEST_NOW_AVAILABLE_LABEL = i18n.translate( | |||
export const PRIVATE_AVAILABLE_LABEL = i18n.translate( | |||
'xpack.synthetics.monitorList.testNow.available.private', | |||
{ | |||
defaultMessage: 'For now, Test now is disabled for private locations monitors.', | |||
defaultMessage: | |||
'Test now is currently not available for monitors running on Private Locations.', |
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.
@justinkambic do you have a screenshot of how this is being used? My initial reaction is that Test now ... is confusing, but if this tooltip is targeting a button that says "Test now" it might be ok (or we could just say "This is not currently available for...").
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.
@colleenmcginnis if the monitor in the list is not fleet-managed and has a valid config ID, we render a play
button which will kick off the one-off test.
We render the tooltip that uses this label in the case that the monitor is fleet-managed. So if the user created the monitor with the synthetics integration, and it's being handled with elastic agent/fleet rather than the synthetics service, we're not able to test it on demand.
I don't have a screenshot of this and my env isn't configured in a way that I can easily get one, but does that explanation make sense? If a screenshot is still necessary I can see about getting one for you today.
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.
Thanks @justinkambic. What to you think about something like this instead?
You can't currently test monitors running on private locations on demand.
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.
@colleenmcginnis I am attaching screenshots of the three states this play (Test now) button can have. Please see if
You can't currently test monitors running on private locations on demand.
is till valid observing the context in screenshots. Also, we may need to change the copy for the other disabled state similar to this one.
When Test now is not available due to monitor running on Private Loations.
@elasticmachine merge upstream |
368dd2c
to
91a64ba
Compare
@@ -82,7 +82,7 @@ export const TEST_NOW_AVAILABLE_LABEL = i18n.translate( | |||
export const PRIVATE_AVAILABLE_LABEL = i18n.translate( | |||
'xpack.synthetics.monitorList.testNow.available.private', | |||
{ | |||
defaultMessage: 'For now, Test now is disabled for private locations monitors.', | |||
defaultMessage: `You can't currently test monitors running on private locations on demand.`, |
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.
@colleenmcginnis LMK if you'd like to see anything else changed here.
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.
This looks good to me if it looks good to you! 👍
@awahab07 @colleenmcginnis can one/both of you approve if there are no further changes needed? |
@elasticmachine merge upstream |
7f596c1
to
f6b0c1c
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
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
…ations (elastic#137912) * Update copy for test now functionality on private locations. * Update copy based on PR feedback. (cherry picked from commit e7772b1)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ations (elastic#137912) (elastic#138835) * Update copy for test now functionality on private locations. * Update copy based on PR feedback. (cherry picked from commit e7772b1) Co-authored-by: Justin Kambic <[email protected]>
…ations (elastic#137912) * Update copy for test now functionality on private locations. * Update copy based on PR feedback.
Summary
Resolves #137898.
Modifies the copy displayed on a tooltip in the application.