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

Add tooltip above Start Execution button #1802

Merged
merged 10 commits into from
Sep 14, 2023
Merged

Conversation

jamie-suse
Copy link
Contributor

@jamie-suse jamie-suse commented Sep 11, 2023

Description

This PR present 3 main visual changes:

Checks Selection tooltip

Adds a tooltip above the Start Execution button in the Cluster Settings and Host Checks Selection pages that says:

Click Start Execution or wait for Trento to periodically run checks.

If no checks or no new checks are saved on the page, this tooltip does not appear.

Host Details page tooltip

This PR also adds the tooltip for

Select some Checks first!

that appears below the Start Execution button in the Host Details page; in order ot align the behaviour as seen in the Cluster Details page.

Align Cluster Details page

This PR also aligns the styling of the Start Execution button in the Cluster Details page with the rest of the app.

How was this tested?

Added assertions in React component tests to test appearance/disappearance of tooltip.

Updated Storybooks.

@jamie-suse jamie-suse added enhancement New feature or request javascript Pull requests that update Javascript code labels Sep 11, 2023
@jamie-suse
Copy link
Contributor Author

jamie-suse commented Sep 11, 2023

One thing to note is that if you save some new checks in Host Checks Selection, then click the Start Execution button, the tooltip disappears as intended. After if you then navigate back to Host Details page, the tooltip will appear again - instead of being hidden because the user has already executed the checks.

Do you think this is a big issue?

Screencast.from.2023-09-11.12-54-19.webm

@jamie-suse jamie-suse marked this pull request as ready for review September 11, 2023 10:19
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Great job so far!

Just a quick recap of the offline discussion:

  • host check selection and cluster check selection should have the same ux/ui regarding the tooltip
  • host detail and cluster detail should have the same ux/ui (ie Select some checks first tooltip and should the Click Start Execution or wait for ... tooltip be there in that page?)
  • enabled/disabled button ui consitency between cluster/host detail/selection

@jamie-suse jamie-suse force-pushed the checks-execution-tooltip branch from d308f77 to 28fa39c Compare September 12, 2023 15:26
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

Just a couple things then we can merge

@@ -23,6 +24,7 @@ function HostChecksSelection({
onSelectedChecksChange,
hostChecksExecutionEnabled,
onStartExecution = () => {},
checksExecutionTooltipVisible,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename this to isExecutionTooltipVisible?

@@ -29,6 +29,8 @@ function HostSettingsPage() {
setSelection(hostSelectedChecks);
}
}, [hostSelectedChecks]);
const [checksExecutionTooltipVisible, setChecksExecutionTooltipVisible] =
Copy link
Contributor

Choose a reason for hiding this comment

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

hostSelectedChecks should update itself reactively if I remember correctly, did you check if you actually need a state here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍 removed this

@jamie-suse jamie-suse force-pushed the checks-execution-tooltip branch 2 times, most recently from 3fdf8a6 to 4e7cbc7 Compare September 13, 2023 14:09
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

@jamie-suse jamie-suse force-pushed the checks-execution-tooltip branch from 4e7cbc7 to b4ff564 Compare September 14, 2023 10:48
@jamie-suse jamie-suse merged commit 872a953 into main Sep 14, 2023
@jamie-suse jamie-suse deleted the checks-execution-tooltip branch September 14, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code
Development

Successfully merging this pull request may close these issues.

3 participants