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.

Sign up for GitHub

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

NumberInput: add option to allow only positive integers #206

Closed
wants to merge 1 commit into from
Closed

NumberInput: add option to allow only positive integers #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/lib/forms/widgets/text/Input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export default class Input extends Component {
description,
disabled,
type,
onKeyPress,
} = this.props;

return (
Expand All @@ -27,6 +28,7 @@ export default class Input extends Component {
label={<FieldLabel htmlFor={fieldPath} icon={icon} label={label} />}
placeholder={placeholder}
type={type}
onKeyPress={onKeyPress}
/>
);
}
Expand All @@ -41,11 +43,13 @@ Input.propTypes = {
required: PropTypes.bool,
disabled: PropTypes.bool,
type: PropTypes.string,
onKeyPress: PropTypes.func,
};

Input.defaultProps = {
icon: undefined,
required: false,
disabled: false,
type: "input",
onKeyPress: undefined,
};
54 changes: 53 additions & 1 deletion src/lib/forms/widgets/text/NumberInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,57 @@
import React from "react";
import Input from "./Input";
import PropTypes from "prop-types";

const NumberInput = (props) => {
const {
fieldPath,
label,
placeholder,
description,
icon,
required,
disabled,
type,
positiveIntegerOnly,
Copy link
Contributor

@kpsherva kpsherva Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is bypassing validation (backend or frontend) in an unorthodox way, without providing any feedback to the user on incorrect input. I think it might be confusing for the user
why not using the min html attribute ?
<input type="number" min="0">
or
<input type="number" step="1" pattern="\d+"> this one should give feedback when you try to type -
this approach can be used without adding a new prop the html attributes should be automatically added by react to the output element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand the first part, the user gets the same feedback as when he tries to type any letter, when he types it, it will not appear in the field.
I agree that we should align the validations between the backend and frontend but it's not correct at the moment either, as in the frontend it's allowed to introduce +- sign and when doing this it will not save the number (although it's displayed in the UI). Here an example of this:

Peek.2023-09-12.18-06.mp4

By doing this change it actually aligns a bit more with the backend validation.

I agree that ideally we also want to remove the + and - sign from the backend validation, but I also think we could tackle that in the future.

I also tried your suggestions without any luck, maybe I am doing something wrong:
Screenshot from 2023-09-12 18-08-39

} = props;
const onKeyPress = (event) => {
if (!/[0-9]/.test(event.key)) {
event.preventDefault();
}
};
return (
<Input
fieldPath={fieldPath}
label={label}
placeholder={placeholder}
description={description}
icon={icon}
required={required}
disabled={disabled}
type={type}
onKeyPress={positiveIntegerOnly && onKeyPress}
/>
);
};

const NumberInput = (props) => <Input {...props} type="number" />;
export default NumberInput;

NumberInput.propTypes = {
fieldPath: PropTypes.string.isRequired,
label: PropTypes.string.isRequired,
placeholder: PropTypes.string.isRequired,
description: PropTypes.string.isRequired,
icon: PropTypes.string,
required: PropTypes.bool,
disabled: PropTypes.bool,
type: PropTypes.string,
positiveIntegerOnly: PropTypes.bool,
};

NumberInput.defaultProps = {
icon: undefined,
required: false,
disabled: false,
type: "number",
positiveIntegerOnly: false,
};
Loading