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

Model Registry: Initial content of model details tab with stub editable fields and tab navigation handling #2760

Conversation

mturley
Copy link
Contributor

@mturley mturley commented Apr 29, 2024

Part of RHOAIENG-2235

Description

Implements most of the "Details" tab of the registered model page. Note that the "Properties" section is not functional and will be implemented in a followup PR. Also note that while the Description and Labels sections have editable functionality implemented, the actual last step of persisting the edits to the API is a stub promise that simply waits 2 seconds in order to show the "saving" state (edits are not allowed while saving). Implementing the real API patch requests will be in a separate PR.

Model details tab:
Screenshot 2024-04-29 at 7 31 32 PM

Description field in edit mode:
Screenshot 2024-04-29 at 7 15 02 PM

Description field disabled while saving (currently a stub 2-second-delay promise):
Screenshot 2024-04-29 at 7 24 38 PM

Labels in edit mode:
Screenshot 2024-04-29 at 7 15 13 PM

Editing an existing label:
Screenshot 2024-04-29 at 7 15 21 PM

Add label modal:
Screenshot 2024-04-29 at 7 24 49 PM
Screenshot 2024-04-29 at 7 24 56 PM

Label editing disabled while saving (currently a stub 2-second-delay promise):
Screenshot 2024-04-29 at 7 25 53 PM

How Has This Been Tested?

  1. Go to Model Registry tab
  2. Click on any registered model in the Registered Models table and then click the Details tab, or click the kebab menu at the right of a registered model and click View details
  3. You will be taken to the model details view shown above
  4. Click between the tabs and note that the URL route is changing and using the new /details and /versions subpaths

Test Impact

Will be added shortly.

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 (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

Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

Ok, so far it's working great, this is going really great 👌

@mturley mturley force-pushed the RHOAIENG-2235-model-details-section branch 2 times, most recently from 29a10b7 to 45bd9c5 Compare April 30, 2024 15:57
@mturley mturley force-pushed the RHOAIENG-2235-model-details-section branch from 45bd9c5 to ea23ed0 Compare April 30, 2024 19:41
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

const [unsavedLabels, setUnsavedLabels] = React.useState(labels);
const [isSavingEdits, setIsSavingEdits] = React.useState(false);

const editUnsavedLabel = (newText: string, index: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that if this is gonna be reused in the details view we can move it into a utils right?
Not needed right now, can be refactor on @dpanshug pr

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolving conversation it can be done in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole EditableLabelsDescriptionListGroup is already ready to be reused anywhere in the UI, will just drop it in for version details

@openshift-ci openshift-ci bot added the lgtm label May 1, 2024
Copy link
Contributor

openshift-ci bot commented May 1, 2024

[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 added the approved label May 1, 2024
@lucferbux
Copy link
Contributor

Ok, so far it's going great! This might unblock the rest of the work!

@openshift-merge-bot openshift-merge-bot bot merged commit 1b56d25 into opendatahub-io:main May 1, 2024
6 checks passed
@mturley mturley deleted the RHOAIENG-2235-model-details-section branch May 1, 2024 16:59
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.

2 participants