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

Node resource table and modals #3565

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions frontend/src/__mocks__/mockHardwareProfile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ export const mockHardwareProfile = ({
{
displayName: 'Memory',
identifier: 'memory',
minCount: 5,
maxCount: 2,
defaultCount: 2,
minCount: '5Gi',
maxCount: '2Gi',
defaultCount: '2Gi',
},
],
description = '',
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/api/k8s/__tests__/hardwareProfiles.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ const data: HardwareProfileKind['spec'] = {
{
displayName: 'Memory',
identifier: 'memory',
minCount: 5,
maxCount: 2,
defaultCount: 2,
minCount: '5Gi',
maxCount: '2Gi',
defaultCount: '2Gi',
},
],
description: 'test description',
Expand Down
49 changes: 49 additions & 0 deletions frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React from 'react';
import { FormSection, Flex, FlexItem, Button } from '@patternfly/react-core';
import { Identifier } from '~/types';
import NodeResourceTable from './nodeResource/NodeResourceTable';
import ManageNodeResourceModal from './nodeResource/ManageNodeResourceModal';

type ManageNodeResourceSectionProps = {
nodeResources: Identifier[];
setNodeResources: (identifiers: Identifier[]) => void;
};

export const ManageNodeResourceSection: React.FC<ManageNodeResourceSectionProps> = ({

Check warning on line 12 in frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx#L12

Added line #L12 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

This is a single file, maybe consider exporting it as default like other components?

nodeResources,
setNodeResources,
}) => {
const [isNodeResourceModalOpen, setIsNodeResourceModalOpen] = React.useState<boolean>(false);
return (

Check warning on line 17 in frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx#L15-L17

Added lines #L15 - L17 were not covered by tests
<>
<FormSection
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode Dec 18, 2024

Choose a reason for hiding this comment

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

In the mockup, there is a description under this section title. Also, the button is a link style below the table but not next to the section title. However, the description is missed here in this PR and the button becomes the secondary button next to the title.

I am OK with changing the edit and delete icon to the kebab menu. However, I am not sure about the description and also the add button because it can affect how we show the empty state.

Can you check and help confirm that @vconzola?

Screenshot 2024-12-18 at 5 16 54 PM

Choose a reason for hiding this comment

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

Please implement as shown in the mockup including the description, the action icons in place of a kebab and the link button for Add resource. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

@dpanshug Please refer to the implementation here

Copy link
Member

Choose a reason for hiding this comment

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

this is not the latest mocks. did you verify with the latest

Copy link
Member

Choose a reason for hiding this comment

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

title={
<Flex>
<FlexItem>Node resources</FlexItem>
<FlexItem>
<Button
variant="secondary"
onClick={() => setIsNodeResourceModalOpen(true)}

Check warning on line 26 in frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx#L26

Added line #L26 was not covered by tests
data-testid="add-node-resource-button"
>
Add resource
</Button>
</FlexItem>
</Flex>
}
>
<NodeResourceTable
nodeResources={nodeResources}
onUpdate={(newIdentifiers) => setNodeResources(newIdentifiers)}

Check warning on line 37 in frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx#L37

Added line #L37 was not covered by tests
/>
</FormSection>
{isNodeResourceModalOpen ? (
<ManageNodeResourceModal
onClose={() => setIsNodeResourceModalOpen(false)}
onSave={(identifier) => setNodeResources([...nodeResources, identifier])}

Check warning on line 43 in frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx#L41-L43

Added lines #L41 - L43 were not covered by tests
nodeResources={nodeResources}
/>
) : null}

Check warning on line 46 in frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/ManageNodeResourceSection.tsx#L46

Added line #L46 was not covered by tests
</>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import * as React from 'react';
import { FormGroup, FormHelperText, HelperText, HelperTextItem } from '@patternfly/react-core';
import MemoryField from '~/components/MemoryField';
import CPUField from '~/components/CPUField';
import NumberInputWrapper from '~/components/NumberInputWrapper';

type CountFormFieldProps = {
label: string;
fieldId: string;
size: number | string;
setSize: (value: number | string) => void;
identifier: string;
errorMessage?: string;
isValid?: boolean;
};

const CountFormField: React.FC<CountFormFieldProps> = ({
label,
fieldId,
size,
setSize,
identifier,
errorMessage,
isValid = true,
}) => {
const renderInputField = () => {
switch (identifier) {
case 'cpu':
return <CPUField onChange={(value) => setSize(value)} value={size} />;
case 'memory':
return <MemoryField onChange={(value) => setSize(value)} value={String(size)} />;
default:
return (

Check warning on line 33 in frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx#L18-L33

Added lines #L18 - L33 were not covered by tests
<NumberInputWrapper
min={0}
value={Number(size)}
onChange={(value) => {
if (value) {
setSize(value);

Check warning on line 39 in frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx#L37-L39

Added lines #L37 - L39 were not covered by tests
}
}}
/>
);
}
};

return (

Check warning on line 47 in frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx#L47

Added line #L47 was not covered by tests
<FormGroup label={label} fieldId={fieldId} data-testid={fieldId}>
{renderInputField()}
{!isValid && errorMessage && (
<FormHelperText>

Check warning on line 51 in frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/CountFormField.tsx#L50-L51

Added lines #L50 - L51 were not covered by tests
<HelperText>
<HelperTextItem data-testid={`${fieldId}-error`} variant="error">
{errorMessage}
</HelperTextItem>
</HelperText>
</FormHelperText>
)}
</FormGroup>
);
};

export default CountFormField;
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import React from 'react';
import { Modal } from '@patternfly/react-core/deprecated';
import DashboardModalFooter from '~/concepts/dashboard/DashboardModalFooter';
import { Identifier } from '~/types';
import useGenericObjectState from '~/utilities/useGenericObjectState';
import { CPU_UNITS, MEMORY_UNITS_FOR_SELECTION, UnitOption } from '~/utilities/valueUnits';
import { EMPTY_IDENTIFIER } from './const';
import NodeResourceForm from './NodeResourceForm';
import { validateDefaultCount, validateMinCount } from './utils';

type ManageNodeResourceModalProps = {
onClose: () => void;
existingIdentifier?: Identifier;
onSave: (identifier: Identifier) => void;
nodeResources: Identifier[];
};

const ManageNodeResourceModal: React.FC<ManageNodeResourceModalProps> = ({
onClose,
existingIdentifier,
onSave,
nodeResources,
}) => {
const [identifier, setIdentifier] = useGenericObjectState<Identifier>(
existingIdentifier || EMPTY_IDENTIFIER,

Check warning on line 25 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L19-L25

Added lines #L19 - L25 were not covered by tests
);

const [unitOptions, setUnitOptions] = React.useState<UnitOption[]>();

Check warning on line 28 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L28

Added line #L28 was not covered by tests

const isUniqueIdentifier = React.useMemo(() => {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to use useMemo on a boolean IMO, it's always reference-stable.

if (existingIdentifier) {
return true;

Check warning on line 32 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L30-L32

Added lines #L30 - L32 were not covered by tests
}
Comment on lines +31 to +33
Copy link
Member

Choose a reason for hiding this comment

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

Why are you not checking the identifier when editing? If I am editing a custom identifier, then it could be duplicated to another one, right? Try to create 2 identifiers, then edit one to change the identifier to the other and see if it throws the error.

Copy link
Member

Choose a reason for hiding this comment

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

In another word, this check only needs to be

const isUniqueIdentifier =
    identifier.identifier === existingIdentifier?.identifier ||
    !nodeResources.some((i) => i.identifier === identifier.identifier);

return !nodeResources.some((i) => i.identifier === identifier.identifier);

Check warning on line 34 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L34

Added line #L34 was not covered by tests
}, [existingIdentifier, identifier.identifier, nodeResources]);

React.useEffect(() => {
switch (identifier.identifier) {
case 'cpu':
setUnitOptions(CPU_UNITS);
break;
case 'memory':
setUnitOptions(MEMORY_UNITS_FOR_SELECTION);
break;
default:
setUnitOptions(undefined);

Check warning on line 46 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L37-L46

Added lines #L37 - L46 were not covered by tests
}
}, [identifier]);

const isButtonDisabled = React.useMemo(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, no need to use useMemo

const isValidCounts = unitOptions
? validateDefaultCount(identifier, unitOptions) && validateMinCount(identifier, unitOptions)
: true;

Check warning on line 53 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L50-L53

Added lines #L50 - L53 were not covered by tests

return (
!identifier.displayName || !identifier.identifier || !isUniqueIdentifier || !isValidCounts

Check warning on line 56 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L55-L56

Added lines #L55 - L56 were not covered by tests
);
}, [identifier, unitOptions, isUniqueIdentifier]);

const handleSubmit = () => {
onSave(identifier);
onClose();

Check warning on line 62 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L60-L62

Added lines #L60 - L62 were not covered by tests
};

return (

Check warning on line 65 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L65

Added line #L65 was not covered by tests
<Modal
title={existingIdentifier ? 'Edit resource' : 'Add resource'}

Check warning on line 67 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L67

Added line #L67 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

Should be Edit node resource and Add node resource here.

variant="medium"
isOpen
onClose={onClose}
footer={
<DashboardModalFooter
submitLabel={existingIdentifier ? 'Update' : 'Add'}

Check warning on line 73 in frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/ManageNodeResourceModal.tsx#L73

Added line #L73 was not covered by tests
onSubmit={handleSubmit}
onCancel={onClose}
isSubmitDisabled={isButtonDisabled}
/>
}
>
<NodeResourceForm
identifier={identifier}
setIdentifier={setIdentifier}
unitOptions={unitOptions}
existingIdentifier={!!existingIdentifier}
isUniqueIdentifier={isUniqueIdentifier}
/>
</Modal>
);
};

export default ManageNodeResourceModal;
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import React from 'react';
import { TextInput, FormGroup, Form } from '@patternfly/react-core';
import { Identifier } from '~/types';
import { UpdateObjectAtPropAndValue } from '~/pages/projects/types';
import { UnitOption } from '~/utilities/valueUnits';
import { validateDefaultCount, validateMinCount } from './utils';
import CountFormField from './CountFormField';

type NodeResourceFormProps = {
identifier: Identifier;
setIdentifier: UpdateObjectAtPropAndValue<Identifier>;
unitOptions?: UnitOption[];
existingIdentifier?: boolean;
isUniqueIdentifier?: boolean;
};

const NodeResourceForm: React.FC<NodeResourceFormProps> = ({
identifier,
setIdentifier,
unitOptions,
existingIdentifier,
isUniqueIdentifier,
}) => {
const [validated, setValidated] = React.useState<'default' | 'error' | 'success'>('default');

Check warning on line 24 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L18-L24

Added lines #L18 - L24 were not covered by tests

React.useEffect(() => {
setValidated(isUniqueIdentifier ? 'default' : 'error');

Check warning on line 27 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L26-L27

Added lines #L26 - L27 were not covered by tests
}, [isUniqueIdentifier]);
Comment on lines +24 to +28
Copy link
Member

Choose a reason for hiding this comment

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

You don't need these at all:

Suggested change
const [validated, setValidated] = React.useState<'default' | 'error' | 'success'>('default');
React.useEffect(() => {
setValidated(isUniqueIdentifier ? 'default' : 'error');
}, [isUniqueIdentifier]);
const validated = isUniqueIdentifier ? 'default' : 'error';


return (

Check warning on line 30 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L30

Added line #L30 was not covered by tests
<Form>
<FormGroup label="Resource label" fieldId="resource-label">
Copy link
Member

Choose a reason for hiding this comment

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

Should add isRequired

<TextInput
value={identifier.displayName || ''}
onChange={(_, value) => setIdentifier('displayName', value)}

Check warning on line 35 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L34-L35

Added lines #L34 - L35 were not covered by tests
/>
</FormGroup>

<FormGroup label="Resource identifier" fieldId="resource-identifier">
Copy link
Member

Choose a reason for hiding this comment

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

Should add isRequired

<TextInput
value={identifier.identifier || ''}
onChange={(_, value) => setIdentifier('identifier', value)}

Check warning on line 42 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L41-L42

Added lines #L41 - L42 were not covered by tests
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode Dec 18, 2024

Choose a reason for hiding this comment

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

This field should also be disabled if it's CPU or RAM based on what I saw in the mockups (Or probably disable it based on the identifier field, WDYT? @Gkrumbach07 )

isDisabled={
existingIdentifier &&
(identifier.identifier === 'cpu' || identifier.identifier === 'memory')

Check warning on line 45 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L44-L45

Added lines #L44 - L45 were not covered by tests
Comment on lines +44 to +45
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be just:

Suggested change
existingIdentifier &&
(identifier.identifier === 'cpu' || identifier.identifier === 'memory')
existingIdentifier?.identifier === 'cpu' || existingIdentifier?.identifier === 'memory'

}
validated={validated}
Copy link
Member

Choose a reason for hiding this comment

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

Missed error message here? Another resource with the same identifier already exists. The resource identifier must be unique.

/>
</FormGroup>

<CountFormField
label="Default"
fieldId="default"
identifier={identifier.identifier}
size={identifier.defaultCount}
setSize={(value) => setIdentifier('defaultCount', value)}
isValid={unitOptions ? validateDefaultCount(identifier, unitOptions) : true}

Check warning on line 57 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L56-L57

Added lines #L56 - L57 were not covered by tests
errorMessage="Default must be equal to or between the minimum and maximum allowed limits."
/>

<CountFormField
label="Minimum allowed"
fieldId="minimum-allowed"
identifier={identifier.identifier}
size={identifier.minCount}
setSize={(value) => setIdentifier('minCount', value)}
isValid={unitOptions ? validateMinCount(identifier, unitOptions) : true}

Check warning on line 67 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L66-L67

Added lines #L66 - L67 were not covered by tests
errorMessage="Minimum allowed value cannot exceed the maximum allowed value."
/>

<CountFormField
label="Default"
fieldId="default"
Comment on lines +72 to +73
Copy link
Member

Choose a reason for hiding this comment

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

These are maximum allowed.

identifier={identifier.identifier}
size={identifier.maxCount}
setSize={(value) => setIdentifier('maxCount', value)}

Check warning on line 76 in frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/hardwareProfiles/nodeResource/NodeResourceForm.tsx#L76

Added line #L76 was not covered by tests
/>
</Form>
);
};
export default NodeResourceForm;
Loading
Loading