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

Show tooltip on disabled 'Deploy model' button when no project is selected #2283

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

mturley
Copy link
Contributor

@mturley mturley commented Dec 6, 2023

Closes https://issues.redhat.com/browse/RHOAIENG-544

Description

Adds a tooltip that appears on the 'Deploy model' button on the global Model Serving page if the button is disabled because no project is selected.

Screenshot 2023-12-06 at 7 54 07 PM

The tooltip is shown here in the empty state when there are no deployed models, but the ServeModelButton component used there is the same one used above the table in the non-empty state of the page, so the tooltip implementation affects both views.

cc @vconzola

How Has This Been Tested?

Viewing the Model Serving page with no project selected, and observing that the disabled button has the new tooltip. Added Cypress test, see below.

Test Impact

Added to existing Cypress tests for the Model Serving page. The new test visits the all-projects view of the page, triggers a mouse hover event on the button and checks that the tooltip is visible.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Dec 6, 2023
Copy link
Contributor

openshift-ci bot commented Dec 6, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mturley mturley marked this pull request as ready for review December 7, 2023 01:01
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Dec 7, 2023
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

/lgtm

@vconzola
Copy link

LGTM.

@mturley
Copy link
Contributor Author

mturley commented Dec 11, 2023

Marking draft briefly while I add some tests

@mturley mturley marked this pull request as draft December 11, 2023 18:21
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Dec 11, 2023
@andrewballantyne
Copy link
Member

Marking draft briefly while I add some tests

FWIW @mturley you can always just prefix your PR name with [WIP] and it will slap a label on it same as going into draft

@mturley
Copy link
Contributor Author

mturley commented Dec 11, 2023

Good to know, thanks!

@mturley mturley changed the title Show tooltip on disabled 'Deploy model' button when no project is selected [WIP] Show tooltip on disabled 'Deploy model' button when no project is selected Dec 11, 2023
@mturley mturley changed the title [WIP] Show tooltip on disabled 'Deploy model' button when no project is selected Show tooltip on disabled 'Deploy model' button when no project is selected Dec 11, 2023
@mturley mturley marked this pull request as ready for review December 11, 2023 20:27
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Dec 11, 2023
@mturley
Copy link
Contributor Author

mturley commented Dec 11, 2023

Added a Cypress test here and marked ready for review again.

@christianvogt
Copy link
Contributor

/retest

Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 11, 2023
Copy link
Contributor

openshift-ci bot commented Dec 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DaoDaoNoCode, lucferbux

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mturley
Copy link
Contributor Author

mturley commented Dec 11, 2023

Who gets to press the big green button ✅👀

@lucferbux
Copy link
Contributor

@mturley We tend to rebase from main rather than merge to keep the git history clean, and if you can clean up the commit history would be amazing (we are changing our flow now, so maybe we could squash in the future) but for now we try to enforce the least amount of commits per PR!
Once we have that, I'll lgtm it again, and prow should auto merge it!

…ected

Signed-off-by: Mike Turley <mike.turley@alum.cs.umass.edu>
@mturley
Copy link
Contributor Author

mturley commented Dec 12, 2023

@lucferbux got it, sounds good! Just squashed it into one rebased commit and force-pushed. should be good now.

@lucferbux
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 12, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit fd00e27 into opendatahub-io:main Dec 12, 2023
6 checks passed
@mturley mturley deleted the issue-2165 branch December 12, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants