-
Notifications
You must be signed in to change notification settings - Fork 163
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
Make content updates for project UI screens #2649
Make content updates for project UI screens #2649
Conversation
Hi @thatblindgeye. Thanks for your PR. I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
73fd647
to
bb70586
Compare
bb70586
to
4760993
Compare
/ok-to-test |
@simrandhaliw just FYI in case you would like to check the content updates |
Tagging @jgiardino in this PR in case she'd like to review the content updates too. |
2b71619
to
d2389d3
Compare
frontend/src/pages/modelServing/screens/projects/ModelServingPlatform.tsx
Show resolved
Hide resolved
d2389d3
to
4b6175f
Compare
4b6175f
to
1f8d91a
Compare
frontend/src/pages/modelServing/screens/projects/ModelServingPlatform.tsx
Show resolved
Hide resolved
I reviewed contents in the latest poc where possible. For contents I couldn't check in the POC, I was able to check if they were present in this PR. I left comments inline regarding a few cases that don't match what I expected. But there is one use case that I couldn't see any contents for in this PR related to the models empty state when both serving platforms are enabled. |
3677926
to
b3e1290
Compare
Everything seems to be good. I think the updated logic for the pipelines card looks good but I can’t really confirm without seeing the UI. 😉 Thank you for making these updates!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
frontend/src/pages/modelServing/screens/projects/ModelServingPlatform.tsx
Outdated
Show resolved
Hide resolved
b3e1290
to
9845f06
Compare
9845f06
to
77de567
Compare
frontend/src/pages/projects/screens/detail/overview/trainModels/PipelinesCard.tsx
Outdated
Show resolved
Hide resolved
Make content updates for Project UI screen Make content updates for Project UI screens Make content updates for project UI screens Additional updates Add empty state for both platforms not enabled Update verbiage for data connections emptystate Updated multimodel verbiage for serve section Remove unneccessary logicals Conditionally render deploy model button in toolbar Fix verbiage Update based on missing update comments in document Remove unnecessary key prop Revert to use Alert component
77de567
to
7484aa9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeff-phillips-18, 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 |
Closes: https://issues.redhat.com/browse/RHOAIENG-4978
Closes: RHOAIENG-4846
Description
Updates to project UI screens were required per a content updates Google doc linked in the ticket above
How Has This Been Tested?
Ran unit and cypress tests locally, also visually when running the dashboard locally
Test Impact
N/A
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main