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

Revamp empty state in Model Serving Global #1835

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

lucferbux
Copy link
Contributor

@lucferbux lucferbux commented Sep 20, 2023

Description

Closes #1091
(Re opening this again since i merge it accidentally)

How Has This Been Tested?

  1. Delete all Model Servers in all projects
  2. Navigate to the Model Serving global page
  3. Check that the empty state is changed
Screenshot 2023-09-27 at 12 37 30

Test Impact

Added coverage for empty state

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

@lucferbux
Copy link
Contributor Author

@lucferbux here's some updated text for this empty state:

"No deployed models yet To get started, deploy a model from the Models and model servers section of a project. [Select a project]"

As for the layout questions, I'm not sure - maybe @vconzola would have an opinion :-)

Bringin this conversation here too @vconzola @kaedward

@vconzola
Copy link

@lucferbux What layout questions do you have?

@lucferbux
Copy link
Contributor Author

@lucferbux What layout questions do you have?

Ah, it's just that changing the button from primary to link removes the top padding, so "Go to the project pages" is closer to the body than other EmptyStates components that we have (even the older one). I would say that's by design but I just want the feedback from UX to make sure if it's ok.

@vconzola
Copy link

@lucferbux It's probably not by design. :-) The PF examples on the website only show empty states with primary buttons, but I understand why we want to use a link button here. So please add some padding to make the spacing consistent with the other empty states.

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.

Is this what I should be seeing? If yes, can you update the screenshot? This looks different from the screenshot in the PR description.
Screenshot 2023-09-26 at 4 48 20 PM

@lucferbux
Copy link
Contributor Author

Is this what I should be seeing? If yes, can you update the screenshot? This looks different from the screenshot in the PR description. Screenshot 2023-09-26 at 4 48 20 PM

I need to add some UX feedback, once i get that I'll update the screenshots!

@lucferbux
Copy link
Contributor Author

Ok @vconzola @kaedward I've updated the layout adding a margin of 32, which is the default with primary button in an EmptyState. You can see it here:

For primary Empty State

Screenshot 2023-09-27 at 12 39 39 Screenshot 2023-09-27 at 12 32 09

For link Empty State
Screenshot 2023-09-27 at 12 40 13
Screenshot 2023-09-27 at 12 37 30

Let me know if this is ok!

@lucferbux
Copy link
Contributor Author

This should be ready to go thanks @manaswinidas @christianvogt let me know if it's ok this way cc @DaoDaoNoCode

@lucferbux
Copy link
Contributor Author

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@openshift-ci openshift-ci bot merged commit 5265f6e into opendatahub-io:main Oct 10, 2023
3 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.

[Bug]: Create Model Server redirects to Data Science Project Screen
6 participants