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

Add hardware profile table #3573

Merged

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Dec 12, 2024

JIRA: https://issues.redhat.com/browse/RHOAIENG-16254

Description

Added the table for the hardware profiles.

Main table:
Screenshot 2024-12-12 at 3 59 24 PM

Expandable sections:
There are 3 tables inside, if there is no data in the table, the corresponding nested table will be hidden.
Screenshot 2024-12-12 at 4 00 13 PM

Filters:
You can filter by Name and Enable columns.
Screenshot 2024-12-12 at 4 01 27 PM
Screenshot 2024-12-12 at 4 02 05 PM

Disable hardware profile modal:
Screenshot 2024-12-12 at 4 03 00 PM

Delete modal:
Screenshot 2024-12-12 at 4 03 20 PM

How Has This Been Tested?

  1. Create some hardware profiles CRs on the clusters in the opendatahub namespace
  2. Try to set the empty array to nodeSelectors, identifiers and tolerations fields in spec to see how the expandable section shows the tables
  3. Try to toggle the enable switch to see the modal and how it performs
  4. Try the delete function
  5. Try the filters, make sure it can filter by the name and enable status

Test Impact

Added cypress tests for this page.

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

@DaoDaoNoCode
Copy link
Member Author

@vconzola Hi, can you check the screenshots?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dpanshug I prefer to keep everything in the root const.ts file because we use different names for the column variables. Just let you know in advance so it's easier for you to rebase if needed.

@vconzola
Copy link

@DaoDaoNoCode A couple comments questions...

  1. What font size are you using for the section headers (Node resources, Node selectors, Tolerations)? They look kinda big.
  2. We should not require confirmation when disabling a profile. We don't confirm when disabling other resources, like notebook images and serving runtimes.
  3. Let's change the column header to "Enabled" instead of "Enable".
  4. All of the content will need a review by @kaedward. Do you want to do that now or after the code is merged?

@DaoDaoNoCode
Copy link
Member Author

DaoDaoNoCode commented Dec 16, 2024

@vconzola Hi, thanks for the feedback.

  1. I am using h6 variant for Title component, which I believe size is md here. This is the smallest size of the Title component, I can change it to normal Text and add bold style manually if you think it's too big now.
  2. I can remove this disabled modal. I saw it when disabling the accelerator profiles so I just copied what they do
Screenshot 2024-12-16 at 9 20 15 AM
  1. Will do
  2. I prefer to get the content reviewed after all the codes are merged

@DaoDaoNoCode
Copy link
Member Author

@vconzola I addressed the feedback as suggested. Can you check it again?

Screenshot 2024-12-16 at 9 49 59 AM

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 94.23077% with 9 lines in your changes missing coverage. Please review.

Project coverage is 85.25%. Comparing base (515dbef) to head (55bdf24).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...s/hardwareProfiles/HardwareProfileEnableToggle.tsx 82.60% 4 Missing ⚠️
...es/hardwareProfiles/DeleteHardwareProfileModal.tsx 85.00% 3 Missing ⚠️
...ages/hardwareProfiles/HardwareProfilesTableRow.tsx 92.59% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3573      +/-   ##
==========================================
+ Coverage   85.17%   85.25%   +0.07%     
==========================================
  Files        1380     1390      +10     
  Lines       31542    31697     +155     
  Branches     8818     8853      +35     
==========================================
+ Hits        26865    27022     +157     
+ Misses       4677     4675       -2     
Files with missing lines Coverage Δ
frontend/src/api/k8s/hardwareProfiles.ts 88.46% <100.00%> (+5.85%) ⬆️
frontend/src/components/FilterToolbar.tsx 94.59% <ø> (ø)
frontend/src/k8sTypes.ts 100.00% <ø> (ø)
...nd/src/pages/hardwareProfiles/HardwareProfiles.tsx 87.50% <100.00%> (+87.50%) ⬆️
...c/pages/hardwareProfiles/HardwareProfilesTable.tsx 100.00% <100.00%> (ø)
...pages/hardwareProfiles/HardwareProfilesToolbar.tsx 100.00% <100.00%> (ø)
frontend/src/pages/hardwareProfiles/const.ts 100.00% <100.00%> (ø)
...ardwareProfiles/nodeResource/NodeResourceTable.tsx 100.00% <100.00%> (ø)
...rdwareProfiles/nodeSelector/NodeSelectorsTable.tsx 100.00% <100.00%> (ø)
...s/hardwareProfiles/toleration/TolerationsTable.tsx 100.00% <100.00%> (ø)
... and 4 more

... and 6 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 515dbef...55bdf24. Read the comment docs.

@vconzola
Copy link

Thanks @DaoDaoNoCode This LGTM. I think it looks better with the smaller font for the section headers. Regarding the confirmation dialog, leave it out for now. I've started a thread within the UXD channel to talk about this, because we're inconsistent in different parts of the UI currently. I'll let you know what we decide, but I don't think that should block you from getting your PR merged, right?

Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

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

/lgtm

looks great

Copy link
Contributor

openshift-ci bot commented Dec 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gkrumbach07

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 065da14 into opendatahub-io:main Dec 17, 2024
6 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