-
Notifications
You must be signed in to change notification settings - Fork 55
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 views for Model Versions and Model Details #409
Conversation
}); | ||
|
||
it('Model versions table', () => { | ||
// TODO: Uncomment when we fix finding dropdown items |
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.
Note that this test was edited slightly to avoid any dropdown checks
137087e
to
ffaa823
Compare
<Route | ||
path={ModelVersionsTab.DETAILS} | ||
element={<ModelVersions tab={ModelVersionsTab.DETAILS} empty={false} />} | ||
/> |
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.
Just to disclose the private conversation, there will be a follow up to add the archive view here.
isEditing = isAddRow, | ||
keyValuePair = { key: '', value: '' }, | ||
deleteProperty = () => Promise.resolve(), | ||
allExistingKeys, |
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.
Take a look at the latest version, it seems they've added a new prop to detect if the model is archived: opendatahub-io/odh-dashboard#3206
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.
That is addressed already in the PR that will follow this one
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
I've left a couple of nits but some of them should be addressed in the follow up PR, current one is working as expected.
@lucferbux: changing LGTM is restricted to collaborators In response to this:
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. |
Signed-off-by: Griffin-Sullivan <[email protected]>
ffaa823
to
7e19ff6
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.
Tested locally and features all work great!!
Thanks Griffin
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexcreasy, 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 |
Description
Adding all Screens, Components, and Cypress tests for Model details and versions table. I've also added a Jest unit test for some helpers that were added in a previous PR. Also note a small addition to our tsconfig to get around an issue I saw in VS Code.
How Has This Been Tested?
Manually testing the UI in mock mode
Jest and Cypress tests
Merge criteria:
DCO
check)If you have UI changes