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

Improve error handling and edge cases of UnitInputSelection #119

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

bastian-src
Copy link
Contributor

@bastian-src bastian-src commented Feb 10, 2025

This PR resolves three issues with the React component UnitInputField which is used when typing the resource limits of a Resource Quota.

From error to floor warning

The UnitInputField prints an error if a user wants to type a value which is a float in the corresponding base unit, for example 5.5 MiB for the memory limit.

Instead of showing an error, we introduce a warning which says that the value is rounded to the next lower value (5.0 MiB):

Before

image

New

Screenshot from 2025-02-10 16-40-07


No more natural

Moreover, we introduce a more generic error message if no number is given (removing the word 'natural' as it is quite mathematical/technical):

Before

image

New

Screenshot from 2025-02-10 16-53-57


Actually, it's Mebi

We are internally using MebiBytes (MiB) for memory and disk space. However, the UI stated "MB" and others.

Before

image

New

image

We use binary representation for host resources like
memory and disk space. However, the UI shows "GB" instead
of "GiB" which is actually used.
@bastian-src bastian-src changed the title Introduce rounding warning for unit selection Improve error handling and edge cases of UnitInputSelection Feb 10, 2025
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

diff in webpack/components/ResourceQuotaForm/ResourceQuotaFormConstants.js LGTM; I cannot really comment on the rest. Handing over to Thorben.

Feel free to ping me again once you have it running on nightly if you need someone to test this.

Copy link
Contributor

@Thorben-D Thorben-D left a comment

Choose a reason for hiding this comment

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

The change works well and is a clean solution to the problem.
image

The action is right, ExclamationCircleIcon is not used anywhere. Other than that, all good 🤩

The React component 'UnitInputField' prints an error
if a user wants to type a value which is a float in the
corresponding base unit, for example 5.5 MiB for the memory limit.
Instead of showing an error, we introduce a warning which says
that the value is rounded to the next lower value (5.0 MiB).
@bastian-src bastian-src force-pushed the fix/unit_selection_and_types branch from b1808a5 to 96af3f1 Compare February 11, 2025 13:24
* Add snapshots covering default params and disabled view
* Add interactive tests to verify onChange and input validation
  functions are called
@bastian-src bastian-src force-pushed the fix/unit_selection_and_types branch from 96af3f1 to e149671 Compare February 11, 2025 13:25
@bastian-src
Copy link
Contributor Author

@maximiliankolb @Thorben-D thanks a lot for your review! 🐧

@Thorben-D I've attached some tests, could you have another look and let me know if you think anything might be missing?

Copy link
Contributor

@Thorben-D Thorben-D left a comment

Choose a reason for hiding this comment

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

Thank you for adding some tests! Test-code looks good as well.

@bastian-src
Copy link
Contributor Author

Thanks @Thorben-D! 🐠

@bastian-src bastian-src merged commit 91804c6 into main Feb 11, 2025
16 checks passed
@bastian-src bastian-src deleted the fix/unit_selection_and_types branch February 12, 2025 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants