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

[Uptime] Fix: Adjust tooltip on Run test button for Private Locations #138863

Conversation

awahab07
Copy link
Contributor

@awahab07 awahab07 commented Aug 16, 2022

Fixes: #137886

Summary

On Uptime -> Monitor Management -> Add Monitor page, Ru test is disabled if only Private Locations are selected without a tooltip. This PR adds a tooltip for this case.

If Add Monitor form is all in valid state and only Private Locations are selected, Run test will be disabled with the following tooltip:
Screenshot 2022-08-16 at 02 14 17
Note that the tooltip content is the same as shown on Uptime -> Monitors page for monitors with only Private Locations.

Note that since we cannot use EuiTooltip because of the scroll issue, A workaround was needed to make EuiPopover be able to be shown on a disabled button. The PR uses onMouseOver and onMouseOut instead of onMouseEnter and onMouseLeave to achieve this.

Additional Enhancements
The PR also adds a tooltip when the test is in-progress and Run test is disabled. It's the same tooltip that is shown on Uptime -> Monitors page when on-demand test is in progress. (see below screenshot)

Additional Fixes
In Addition to the above, the PR fixes the behavior where Run test was running tests on all selected location (both public and private). After the fix, Run test will only run the on-demand test on public locations. (see below screenshot)

Screenshot 2022-08-16 at 02 19 44

@awahab07 awahab07 added bug Fixes for quality problems that affect the customer experience Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes v8.4.0 v8.5.0 labels Aug 16, 2022
@awahab07 awahab07 requested a review from a team as a code owner August 16, 2022 00:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)


<EuiToolTip
position="top"
content={<p>Works on any kind of element &mdash; buttons, inputs, you name it!</p>}
Copy link
Contributor

Choose a reason for hiding this comment

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

What content is this supposed to have?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this further, I think this whole code block isn't supposed to be here.

export const PRIVATE_AVAILABLE_LABEL = i18n.translate(
'xpack.synthetics.monitorList.testNow.available.private',
{
defaultMessage: `You can't currently test monitors running on private locations on demand.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: `You can't currently test monitors running on private locations on demand.`,
defaultMessage: `Test now is currently not available for monitors running on private locations`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The copy comes from here #137912 (comment).

export const TEST_NOW_ARIA_LABEL = i18n.translate(
'xpack.synthetics.monitorList.testNow.AriaLabel',
{
defaultMessage: 'CLick to run test now',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'CLick to run test now',
defaultMessage: 'Click to run test now',

Comment on lines 150 to 157
<EuiToolTip
position="top"
content={<p>Works on any kind of element &mdash; buttons, inputs, you name it!</p>}
>
<EuiButton disabled={true} onClick={() => {}}>
Hover me
</EuiButton>
</EuiToolTip>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<EuiToolTip
position="top"
content={<p>Works on any kind of element &mdash; buttons, inputs, you name it!</p>}
>
<EuiButton disabled={true} onClick={() => {}}>
Hover me
</EuiButton>
</EuiToolTip>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Checked-in the example piece. Thanks for spotting.

@dominiqueclarke dominiqueclarke self-requested a review August 16, 2022 22:59
{onTestNow && (
<EuiFlexItem grow={false}>
{/* Popover is used instead of EuiTooltip until the resolution of https://github.com/elastic/eui/issues/5604 */}
<EuiPopover
repositionOnScroll={true}
ownFocus={false}
initialFocus={''}
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do when a value isn't specified?

Copy link
Contributor Author

@awahab07 awahab07 Aug 17, 2022

Choose a reason for hiding this comment

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

Here we don't want the popover to capture focus when it's opened, to behave like a tooltip. Providing an empty selector is the only option as undefined is the default value.

Without specifying the prop, it focuses the popover, stealing the focus from monitor form.

@dominiqueclarke dominiqueclarke self-requested a review August 17, 2022 01:27
@awahab07 awahab07 force-pushed the fix-adjust-tooltip-on-run-test-for-on-private-locations branch from 3eafdd1 to dc539d0 Compare August 17, 2022 14:10
@awahab07
Copy link
Contributor Author

@dominiqueclarke the suggested changes are ready.

@awahab07 awahab07 force-pushed the fix-adjust-tooltip-on-run-test-for-on-private-locations branch from 941a48f to 651761a Compare August 18, 2022 11:12
@awahab07
Copy link
Contributor Author

@elasticmachine merge upstream

@awahab07
Copy link
Contributor Author

@elasticmachine merge upstream

@dominiqueclarke
Copy link
Contributor

LGTM generally, but I wonder about one thing.

Screen Shot 2022-08-18 at 4 00 24 PM

On prem, before selecting a location, the test now button still displays information about test now. Only when you add a location do you realize you can't actually use test now. I think that for on-prem, we should display that you cannot use test now immediately.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

LGTM

@awahab07
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
synthetics 992.7KB 993.1KB +399.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@awahab07 awahab07 merged commit f3331b9 into elastic:main Sep 21, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.4 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 138863

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 23, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 138863 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 138863 locally

@awahab07
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

awahab07 added a commit to awahab07/kibana that referenced this pull request Sep 26, 2022
…elastic#138863)

* Control popover logic with MouseOver and MouseOut to be able to show popover on a disabled button.

* Run "Test now" only on public locations.

* Show specific tooltip when only private locations are available.

(cherry picked from commit f3331b9)

# Conflicts:
#	x-pack/plugins/synthetics/public/legacy_uptime/components/monitor_management/action_bar/action_bar.tsx
awahab07 added a commit that referenced this pull request Sep 26, 2022
…#138863) (#141778)

* Control popover logic with MouseOver and MouseOut to be able to show popover on a disabled button.

* Run "Test now" only on public locations.

* Show specific tooltip when only private locations are available.

(cherry picked from commit f3331b9)

# Conflicts:
#	x-pack/plugins/synthetics/public/legacy_uptime/components/monitor_management/action_bar/action_bar.tsx
@kibanamachine kibanamachine added v8.4.3 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.4.0 v8.4.3 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Tooltip on "Run test" button if on-prem locations (add/edit monitor page)
5 participants