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

styling tweaks to model catalog cards #3803

Conversation

gitdallas
Copy link
Contributor

@gitdallas gitdallas commented Feb 25, 2025

part of https://issues.redhat.com/browse/RHOAIENG-19203

Description

tweaks to model catalog cards based on discussion with @yih-wang and her latest figma mockups. includes

  • the redhat ui tags icon (replacing the default pf tags icon)
  • some spacing changes around the tags icon
  • put the labels in the card footer to force a little more padding between description and label (vs stack gutter padding)

image
image

How Has This Been Tested?

locally on green cluster

Test Impact

none

Request review criteria:

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

@mturley
Copy link
Contributor

mturley commented Feb 25, 2025

@yih-wang @gitdallas I imagine we should also use this new tag icon on the details page? the old tag icon is used twice there

Screenshot 2025-02-24 at 1 21 33 PM

@gitdallas
Copy link
Contributor Author

gitdallas commented Feb 25, 2025

just to show what that looks like:
image
image

btw - if you notice how my icon within a label seems off centered? i was chatting with coker about that, it doesn't happen on his PC, but happens in mine, both using macos and same version of chrome, incognito etc... can't figure out why it shows up like that for me 😆

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.57%. Comparing base (0a4fd27) to head (c660e48).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
frontend/src/concepts/design/utils.ts 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3803      +/-   ##
==========================================
- Coverage   84.58%   84.57%   -0.01%     
==========================================
  Files        1529     1531       +2     
  Lines       35557    35566       +9     
  Branches     9960     9964       +4     
==========================================
+ Hits        30075    30081       +6     
- Misses       5482     5485       +3     
Files with missing lines Coverage Δ
frontend/src/concepts/design/TypedObjectIcon.tsx 98.13% <100.00%> (+0.05%) ⬆️
frontend/src/images/icons/ModelCatalogIcon.ts 100.00% <100.00%> (ø)
frontend/src/images/icons/RhUiTagIcon.ts 100.00% <100.00%> (ø)
...pages/modelCatalog/components/ModelCatalogCard.tsx 100.00% <100.00%> (ø)
...nd/src/pages/modelCatalog/screens/ModelCatalog.tsx 100.00% <ø> (ø)
...rc/pages/modelCatalog/screens/ModelDetailsPage.tsx 100.00% <100.00%> (ø)
...rc/pages/modelCatalog/screens/ModelDetailsView.tsx 88.88% <ø> (ø)
frontend/src/concepts/design/utils.ts 72.10% <75.00%> (+0.03%) ⬆️

... and 7 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 0a4fd27...c660e48. Read the comment docs.

@yih-wang
Copy link

yih-wang commented Feb 26, 2025

@gitdallas Changes look great!
Some layout details seem off in the 2nd screen of Figma. Sorry I should’ve called all these details out in our discussion yesterday!

  • Add small spacing between the model name and version to give it some breathing room.
    image
  • I know we don’t have many labels now, but for future-proofing, we could use the overflow label group and collapse labels when there are more than three.
    image
  • Reduce the logo size to 40*40 to make the card feel more balanced.
    Edit: I realized the logo doesn't work as I originally suggested. Only the height is fixed at 40px to accommodate various logos. I think it makes sense and I can see it aligns with what we do for the Resources page. But I was confused about why the logo on catalog card looks much bigger than it looks on the Resources page's card. So I looked into it a little bit and found two things we could improve:
    --1) there's a 4px padding for img on the resource card while we don't have. That's the main reason that the logo there is actually smaller than catalog. It would be good if we can address it the same way.
    image
    --2) Since the catalog card is smaller, I think it would look better if we reduce the fixed height from 40px to 36px.

    Utimately, it should look like this
    image

@gitdallas Not sure if that's clear enough. LMK if you want to talk more offline!

@yih-wang
Copy link

For details page, I'm nitpicking based on what's currently pusblished in the green's cluster:

  • Logo should be aligned with the vertical center
    image

  • Add a little more space between logo/text, and name/description as the measurement below
    image

@yih-wang
Copy link

yih-wang commented Feb 26, 2025

One more update - the model catalog icon is just created and shared from the branding team (download it from here). It should swap out the MR icon we're currently using as a placeholder. And we should use 'teal' (the same color as pipeline) for the icon container, check the screenshot below:

[Should be like]
image
[Current look]
image

@gitdallas gitdallas force-pushed the feat/model-catalog-cards-tweaks branch from 21585c1 to 67e05a5 Compare February 26, 2025 16:35
@gitdallas
Copy link
Contributor Author

discussion from the green meeting this morning is to leave most of these for later stories. i did update the page icon/description and icons on the detail page
image

@gitdallas gitdallas force-pushed the feat/model-catalog-cards-tweaks branch from 67e05a5 to f9c7edb Compare February 26, 2025 16:43
@gitdallas gitdallas requested a review from mturley February 26, 2025 22:39
@yih-wang
Copy link

Thank you @gitdallas, looks good!

@gitdallas gitdallas force-pushed the feat/model-catalog-cards-tweaks branch from 0ae663b to b788b37 Compare February 27, 2025 17:53
@mturley
Copy link
Contributor

mturley commented Feb 28, 2025

@yih-wang not sure if I already shared this - I opened https://issues.redhat.com/browse/RHOAIENG-20552 to capture any style nitpicks we should address that aren't showstoppers for MVP. I know it's important to get the details right, we're just at the point now in our 2.19 timeline that it might have to be ok to roll some of the smaller details over as issues to follow up on. I anticipate we can get these and any other style fixes into 2.20 if we can get them laid out in that issue before next sprint. @gitdallas if they're quick we can knock them out asap, but let's make sure all our other deliverables are at least in progress first.

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.

Good stuff, thanks @gitdallas

@gitdallas gitdallas force-pushed the feat/model-catalog-cards-tweaks branch from b788b37 to c660e48 Compare February 28, 2025 18:37
@openshift-ci openshift-ci bot removed the lgtm label Feb 28, 2025
@openshift-ci openshift-ci bot added the lgtm label Feb 28, 2025
Copy link
Contributor

openshift-ci bot commented Feb 28, 2025

[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-merge-bot openshift-merge-bot bot merged commit dfbe669 into opendatahub-io:main Feb 28, 2025
30 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.

3 participants