-
Notifications
You must be signed in to change notification settings - Fork 178
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
Node resource table and modals #3565
Conversation
WIP: needs some clarification around UX and data validation |
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceTableRow.tsx
Outdated
Show resolved
Hide resolved
13a9f2d
to
60d87b6
Compare
fd5b060
to
fd8a726
Compare
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceTableRow.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx
Outdated
Show resolved
Hide resolved
I believe there should be a space between the numbers and the units in the table, e.g., "1 GiB", not "1GiB", bur defer to @kaedward. Also, the units in the table should match the units in the dropdown, e.g., "GiB", not "Gi". |
frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx
Outdated
Show resolved
Hide resolved
Could you also update the other table node table to now use your view only table |
I think all the tables should use |
1abbee0
to
c2f6c05
Compare
frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceTable.tsx
Outdated
Show resolved
Hide resolved
@dpanshug Can you update the add button like we discussed above as well? |
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceTableRow.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceTableRow.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DaoDaoNoCode, 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 |
RHOAIENG-16256
Description
Added table and modal for Node resource for Hardware profile
How Has This Been Tested?
Currently this table and modal are standalone components and are not used anywhere, to tests you can import
<ManageNodeResourceSection />
to any form page, like i have tested this inManageAcceleratorProfile
Test Impact
Add unit tests for validation utility functions
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main
@vconzola