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

Remove decimals from age input box #962

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

abhcs
Copy link
Collaborator

@abhcs abhcs commented Dec 14, 2023

Fix #928 by removing trailing zeros if the value is integral.

The change also allows the user to type a value with up to 16 fractional digits and truncates the value only on blur (16 is chosen arbitrarily, of course). This is the more natural behavior because the inability to see the third fractional digit while it changes the second visible fractional digit is confusing. The simple ant-design InputNumber component with precision = 2 also allows the user to enter many fractional digits and truncates the value on blur.

🤖[deprecated] Generated by Copilot at 0f0d7a9

Summary

🐛🎨🚸

Fixed a formatting bug and improved user typing for float variables in the VariableEditor component. Modified the formatter function in src/pages/household/input/VariableEditor.jsx.

float formatter
bug fixed with more logic
spring cleaning the code

Walkthrough

  • Fixed a bug in the formatter function for float variables to handle integer values and user typing better (link). This change improved the user interface and usability of the VariableEditor component, which allows users to edit the values of policy parameters and household characteristics in the household page.

@anth-volk
Copy link
Collaborator

I'm going to hold off on reviewing this until reviewing #975, as merging this may create merge conflicts, since the prod repo now contains a different input component

@anth-volk
Copy link
Collaborator

@abhcs Now that #975 has been incorporated, would you mind rebasing off master before I review this?

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

Thanks for your changes on this. I get where you're coming from on this, but it looks like isInteger changes whenever the user changes the input to a floating-point value, making the age box merely display a float rounded to two variables. The critical issue here, though, is that age's value_type in the metadata is described as float', not int for some reason. I'm going to open an issue, since it'd be ideal to actually apply universal rules for all int types instead of trying to determine if something's a float or int on the front end.

@abhcs
Copy link
Collaborator Author

abhcs commented Dec 27, 2023

There is an issue open for that: PolicyEngine/policyengine-us#3357. However, I think that age is not an int, because there are policies in the back end that require age to be float. I have mentioned one age variable of this type in #962: ccdf_age_group.

@abhcs
Copy link
Collaborator Author

abhcs commented Dec 27, 2023

This PR fixes #962 in the following sense: the default value, if integral, will not contain decimals.

This PR also addresses another issue: the user can now freely type 12.99999, and the value is rounded on enter or blur. On the live website trying to type 12.9999... will keep incrementing the value in the box due to rounding.

@anth-volk
Copy link
Collaborator

anth-volk commented Dec 27, 2023

You're right, age is, in fact, required to be of type float, thanks for pointing that out. Yeah, this might just be the best workaround possible without altering age in the country packages to be of type int, then requiring variables in the packages to cast age from int to float (though that'd be my preference).

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

This is perhaps the best workaround to our own back-end limitations, and I do appreciate the additional effort to remove decimal processing that could confuse users. Thanks @abhcs.

@anth-volk anth-volk merged commit 0ad75a7 into PolicyEngine:master Dec 27, 2023
2 checks passed
@abhcs abhcs deleted the fix-issue-928 branch December 27, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove decimals from age input box
2 participants