Add the Model Version Details and Model Version Archive pages #428
Conversation
google-oss-prow
bot
requested review from
alexcreasy,
andreyvelich and
ederign
...nts/ui/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/modelVersionArchive.ts
Outdated
Show resolved
Hide resolved
| }); | ||
| }); | ||
|
|
||
| // TODO: Uncomment when we have restoring and archiving mocked |
There was a problem hiding this comment.
Am I correct that we are not able to mock this at the moment with Cypress? Both archiving a model or model version as well as restoring them didn't seem to work.
There was a problem hiding this comment.
mmmm this is interesting, we can track this later, cause it might be an issue with the current code.
Griffin-Sullivan
force-pushed
the
detail-details
branch
2 times, most recently
from
ce9b7e7 to
fc931e8
Compare
Griffin-Sullivan
force-pushed
the
detail-details
branch
2 times, most recently
from
10e58ad to
9fe0712
Compare
Signed-off-by: Griffin-Sullivan <[email protected]>
Griffin-Sullivan
force-pushed
the
detail-details
branch
from
9fe0712 to
a1e62f4
Compare
| @@ -77,7 +77,7 @@ const RegisteredModelsArchiveListView: React.FC<RegisteredModelsArchiveListViewP | |||
| icon={<FilterIcon />} | |||
| /> | |||
| </ToolbarFilter> | |||
| <ToolbarItem variant="label"> | |||
There was a problem hiding this comment.
As a note for anyone wondering why I removed these label variants, they were a port from the PF6 SPIKE PR we have been following which is untested. Turns out if you use the label variant on the ToolbarItem it will add the aria-hidden='true' property. This goes against a11y because we have a search input as a child which is focusable. Removing the label variant removes the aria-hidden prop.
There was a problem hiding this comment.
Ah yes, that's a great point, I'm sorry cause I faced a similar issue and I didn't report it back, dind't ring a bell yesterday when we discussed it.
| @@ -33,11 +34,20 @@ const RegisteredModelTableRow: React.FC<RegisteredModelTableRowProps> = ({ | |||
| const [isRestoreModalOpen, setIsRestoreModalOpen] = React.useState(false); | |||
| const rmUrl = registeredModelUrl(rm.id, preferredModelRegistry?.name); | |||
|
|
|||
| const actions = [ | |||
| const actions: IAction[] = [ | |||
There was a problem hiding this comment.
This change is porting over some code that was added after the last PR
| onCancel={() => setIsArchiveModalOpen(false)} | ||
| onSubmit={() => | ||
| apiState.api | ||
| .patchModelVersion( |
There was a problem hiding this comment.
There's an issue with this that I'm fixing in my PR, we can leave it as it is right now, all bff requests should have the format { data: T }
| @@ -11,11 +11,13 @@ import ModelPropertiesTableRow from '~/app/pages/modelRegistry/screens/ModelProp | |||
|
|
|||
| type ModelPropertiesDescriptionListGroupProps = { | |||
| customProperties: ModelRegistryCustomProperties; | |||
| isArchive?: boolean; | |||
There was a problem hiding this comment.
Great addition, now we are synced.
|
@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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ederign, 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 |
|
/lgtm |
Description
This adds the Model Version Details page, Model Version Archive page, and Registered Model archive page. I've also got some tests for each.
How Has This Been Tested?
Cypress mock testing. See added tests.
Merge criteria:
DCOcheck)If you have UI changes