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

Feat/5463 mr new design refactor #3139

Conversation

gitdallas
Copy link
Contributor

@gitdallas gitdallas commented Aug 29, 2024

closes: https://issues.redhat.com/browse/RHOAIENG-5463

Description

several design changes

new emptystate images:
image
image
image

breadcrumbs/titles/icons:
image
image
image

model location parts
image
with uri
image

view details is back
image
on archived as well
image

How Has This Been Tested?

locally, mr cluster

Test Impact

updated tests where necessary

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • 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.

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

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.18%. Comparing base (3e40d40) to head (83dc079).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...nd/src/components/InlineTruncatedClipboardCopy.tsx 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3139      +/-   ##
==========================================
+ Coverage   85.17%   85.18%   +0.01%     
==========================================
  Files        1251     1252       +1     
  Lines       27436    27486      +50     
  Branches     7280     7304      +24     
==========================================
+ Hits        23369    23415      +46     
- Misses       4067     4071       +4     
Files with missing lines Coverage Δ
frontend/src/concepts/design/utils.ts 89.92% <100.00%> (+0.75%) ⬆️
...ontend/src/concepts/pipelines/NoPipelineServer.tsx 100.00% <ø> (ø)
...rc/pages/modelRegistry/ModelRegistryCoreLoader.tsx 94.44% <ø> (ø)
.../src/pages/modelRegistry/screens/ModelRegistry.tsx 100.00% <ø> (ø)
...es/modelRegistry/screens/ModelRegistrySelector.tsx 61.01% <ø> (ø)
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx 89.28% <100.00%> (+5.95%) ⬆️
...nDetails/ModelVersionRegisteredDeploymentsView.tsx 100.00% <100.00%> (ø)
...try/screens/ModelVersions/ModelVersionListView.tsx 86.66% <100.00%> (+0.62%) ⬆️
...reens/RegisteredModels/RegisteredModelListView.tsx 79.41% <100.00%> (+1.28%) ⬆️
...reens/RegisteredModels/RegisteredModelTableRow.tsx 100.00% <100.00%> (ø)
... and 2 more

... and 12 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e40d40...83dc079. Read the comment docs.

@gitdallas gitdallas force-pushed the feat/5463-mr-new-design-refactor branch from 631998d to ace7c33 Compare August 30, 2024 13:58
@yih-wang
Copy link

yih-wang commented Sep 2, 2024

Hi @gitdallas, thanks for all the updates! It looks good! Just several comments -

Regarding the empty state of the versions table, it looks like the spacing above and below the image is larger than on other screens. I just checked the source file, and it turns out the white spaces of the source image are bigger than in other images... My apologies for that. I'll follow up with the visual designer and see if we can get an image with less white space to you by Tuesday.

Regarding the model location,

  • I can't see how the long text behaves from the screenshots. Just a quick reminder that the text should be truncated by one line if it exceeds the container length.
image
  • For the S3 type of model location, would it be possible to group 'endpoint, region, bucket, and path' all under a single 'Model Location' section and assign the group a smaller spacing value? That would help clearly distinguish it from the following attributes and make the grouping more apparent. Check the mockup here.
    image

@mturley
Copy link
Contributor

mturley commented Sep 3, 2024

@gitdallas checking the model location fields, if the text is longer than the container width it does wrap to multiple lines instead of truncating:

Screenshot 2024-09-03 at 3 00 34 PM

I was looking at the ClipboardCopy component docs though, and I don't think there's a way to make the inline variant truncate to one line. We could instead switch to the expandable variant, but it might be overkill if these values will usually fit on one line. What do you think @yih-wang ? I'm thinking maybe if there's no quick solution here we should create a post-MVP issue to follow up on this.

@gitdallas
Copy link
Contributor Author

@mturley - would something like this be okay? little hacky and you gotta override the oncopy https://codesandbox.io/p/sandbox/red-wind-zdgyqf?file=%2Findex.js%3A20%2C37

@mturley
Copy link
Contributor

mturley commented Sep 3, 2024

Oh, interesting... yeah @gitdallas I think that solution looks good to me. I wonder if we should open an issue with PF to propose adding a prop or other variant for this, like inline-compact-truncate or something. Maybe for now let's factor out a. ClipboardCopyInlineTruncate component in src/components (or whatever you want to name it) and let's put a comment on that inline style line explaining that it's a workaround.

@gitdallas
Copy link
Contributor Author

gitdallas commented Sep 3, 2024

@yih-wang - regarding the "smaller spacing", that feels off to me personally and i don't really notice it myself... if i do, it just seems like some sort of accident. if it's what is decided on, i can do it - i'd just need the numbers

seems sort of like it might make sense to do a left margin, similar to how form sections work. although being a split screen already it's a bit low on horizontal space.

@yih-wang
Copy link

yih-wang commented Sep 4, 2024

@gitdallas thanks for sharing your thoughts! I had the same worry before about whether it was clear enough to group things in different spaces. Seems it definitely needs better ways to convey that. I'd like to think more about this so let's keep it as is. We have another post-MVP issue to add another new field 'source model format version' on this page and we can address this layout enhancement with that together.

@gitdallas
Copy link
Contributor Author

@yih-wang - with truncated clipboard copy:

image

@gitdallas gitdallas force-pushed the feat/5463-mr-new-design-refactor branch from ace7c33 to 2b97dda Compare September 4, 2024 16:14
@gitdallas gitdallas force-pushed the feat/5463-mr-new-design-refactor branch from 2b97dda to a3fb433 Compare September 4, 2024 17:34
@gitdallas gitdallas force-pushed the feat/5463-mr-new-design-refactor branch from a3fb433 to 5ac75a0 Compare September 4, 2024 17:38
Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @gitdallas

Copy link
Contributor

openshift-ci bot commented Sep 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mturley

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 Sep 5, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 6b8bc8f into opendatahub-io:main Sep 5, 2024
8 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.

4 participants