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

[RHOAIENG-7572] Registered Models - Empty View Redesign #3069

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

jpuzz0
Copy link
Contributor

@jpuzz0 jpuzz0 commented Aug 6, 2024

https://issues.redhat.com/browse/RHOAIENG-7572

Description

Updated icon/text for empty state of the registered models list page.

image image

(cc @yih-wang)

How Has This Been Tested?

Manually tested. Can be tested with a model registry that has no registered models.

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 or cypress tests 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.

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

@jpuzz0 jpuzz0 force-pushed the RHOAIENG-7572 branch 2 times, most recently from f966bb4 to ed00095 Compare August 6, 2024 16:02
@yih-wang
Copy link

yih-wang commented Aug 7, 2024

Copy my comment from slack for the tracking purpose:
User will see this empty state in two cases - when no model registry has been created in the current cluster or the user has no access to any of the existing model registries.
In either case, there won't be a context selector for selecting registries.

@jpuzz0
Copy link
Contributor Author

jpuzz0 commented Aug 7, 2024

Copy my comment from slack for the tracking purpose: User will see this empty state in two cases - when no model registry has been created in the current cluster or the user has no access to any of the existing model registries. In either case, there won't be a context selector for selecting registries.

@yih-wang updated & added new screenshots in the description.

@yih-wang
Copy link

yih-wang commented Aug 8, 2024

LGTM, thanks!

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.

According to the JIRA, I think the empty state is for "No model registries" rather than "No registered models in a model registry". I can still see this empty state for "No model registries" I think you have to change this empty state in ModelRegistryCoreLoader instead of ModelRegistry. There is also a TODO: Replace this with a component for empty registries once we have the designs for this one.
Screenshot 2024-08-08 at 1 55 57 PM

@jpuzz0
Copy link
Contributor Author

jpuzz0 commented Aug 8, 2024

According to the JIRA, I think the empty state is for "No model registries" rather than "No registered models in a model registry". I can still see this empty state for "No model registries" I think you have to change this empty state in ModelRegistryCoreLoader instead of ModelRegistry. There is also a TODO: Replace this with a component for empty registries once we have the designs for this one.

@manaswinidas thanks for pointing this out. I've updated the PR with this change.

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.

Works fine now. We need Cypress tests with a new test-id.

@yih-wang
Copy link

yih-wang commented Aug 9, 2024

@jpuzz0 A following comment - The screenshot of the hovering state is removed, but I kinda remember that the button has an underline while hovering. Could you please check whether you're using the link button variant or the inline button variant for this component? The link button variant should be used, and when applied correctly, the link will not have an underline while hovering.

@jpuzz0
Copy link
Contributor Author

jpuzz0 commented Aug 9, 2024

@jpuzz0 A following comment - The screenshot of the hovering state is removed, but I kinda remember that the button has an underline while hovering. Could you please check whether you're using the link button variant or the inline button variant for this component? The link button variant should be used, and when applied correctly, the link will not have an underline while hovering.

@yih-wang I added the screenshot back which shows the underline on hover.

@mturley
Copy link
Contributor

mturley commented Aug 12, 2024

LGTM pending resolution of the isInline prop discussed above

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

Works fine. The empty states pop up fine for no model registry and no registered models in a model registry.

@openshift-ci openshift-ci bot added the lgtm label Aug 13, 2024
Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jpuzz0 !

Copy link
Contributor

openshift-ci bot commented Aug 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: manaswinidas, mturley

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

@openshift-merge-bot openshift-merge-bot bot merged commit 1ade788 into opendatahub-io:main Aug 13, 2024
6 checks passed
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.

5 participants