Skip to content

Conversation

@MariaAga
Copy link
Member

@MariaAga MariaAga commented Feb 4, 2026

WIP as there is an update the vmware page counter usage, and it might be best to use the wrapper instead. anyway after the vmware page changes this pr should deprecate common/forms/NumericInput.js

The component is defined in webpack/assets/javascripts/react_app/components/common/forms/CounterInput/CounterInput.js

It uses rc-input-number library as it's base. Even though the library seems to be still maintained, it would be better to get rid of one more dependency, especially given that PF5 already has similar capabilities: https://v5-archive.patternfly.org/components/number-input/html

Used in create host -> select compute resource vmware -> virtual machines
create host -> select compute resource libvirt -> virtual machines
discovery_rules/new
and plugins

Tested were generated with AI help

Copy link
Contributor

@adamlazik1 adamlazik1 left a comment

Choose a reason for hiding this comment

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

Generally works as expected and looks good. I have few small things to point out.

I'd consider cutting off the decimal part:

Image

But this is not a regression so I will leave that to your judgement.

And maybe units could be added to the warning and error messages too? I see them in the old implementation.

const input = screen.getByRole('spinbutton');
expect(input).toBeDisabled();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
});
});

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks! reformatted the test file completely

@MariaAga
Copy link
Member Author

I'd consider cutting off the decimal part:

I think its out of the scope of the PR since the number comes from compute_resource.max_memory.to_i in app/views/compute_resources_vms/form/libvirt/_base.html.erb which is not really the counter input

And maybe units could be added to the warning and error messages too? I see them in the old implementation.

Which ones are missing?
there are:
✓ should display warning message when value exceeds recommendedMaxValue (13 ms)
✓ should display error message when value exceeds max (13 ms)
✓ should prioritize error over warning when both conditions are met (11 ms)
✓ should clear warning when value becomes valid (15 ms)
✓ should clear error when value becomes valid (14 ms)
✓ should not display error or warning when value is within limits (6 ms)

@MariaAga MariaAga marked this pull request as ready for review February 10, 2026 11:49
@MariaAga MariaAga marked this pull request as draft February 10, 2026 11:50
@adamlazik1
Copy link
Contributor

adamlazik1 commented Feb 10, 2026

I think its out of the scope of the PR since the number comes from compute_resource.max_memory.to_i in app/views/compute_resources_vms/form/libvirt/_base.html.erb which is not really the counter input

Ack, let's not do that then.

Which ones are missing?

The messages are present, but in the old implementation they contained units like MB as well:
image

Comment on lines 25 to 31
useEffect(() => {
deprecate(
'forms/NumericInput',
'Use components/common/forms/CounterInput/CounterInput.js , or NumberInput from @patternfly/react-core instead',
'3.20'
);
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is used on the vmware form then I can't quickly access it but I would expect the output to be slightly botched based on #10852 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, thanks!

@MariaAga
Copy link
Member Author

misread unit as unit tests 🤦‍♀️
Added
image

Copy link
Contributor

@adamlazik1 adamlazik1 left a comment

Choose a reason for hiding this comment

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

LGTM.

Tiny nitpick: Would it be possible to create space between the icon and the error message?

Image

@adamlazik1
Copy link
Contributor

No robottelo/airgun tests appear to reference the CounterInput component so I would say this doesn't break robottelo.

@MariaAga
Copy link
Member Author

added space in common/forms/FormField.js

Copy link
Contributor

@adamlazik1 adamlazik1 left a comment

Choose a reason for hiding this comment

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

LGTM. Verfied on packit build.

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.

2 participants