-
Notifications
You must be signed in to change notification settings - Fork 427
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
Add frontend min-length validation to name field #8829
Conversation
@@ -54,7 +54,8 @@ function TextField({ | |||
type, | |||
value, | |||
onChangeValue, | |||
limit, | |||
minLength = -1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using -1
rather than null
or undefined
as the default value makes the code nicer because you can just do things like if (value.length < minLength)
or >= minLength
without always having to test for === null
or !== null
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using minLength = 0
would also work here, and avoids potential hazards due to using an "invalid" value as a sentinel. By this I mean that some code assumes a length value is >= 0, and behaves unexpectedly given a negative input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works as expected. I added a few notes about choice of state, which don't really matter yet but would do if error conditions became more complex in future.
After using the UI I think that we should change to showing validation errors inline. As it is we show that "something" is wrong if you enter a too-short name and the field loses focus, but don't spell out the exact problem until a submission is triggered. I would suggest to get this version merged first and come back to this later though.
@@ -54,7 +54,8 @@ function TextField({ | |||
type, | |||
value, | |||
onChangeValue, | |||
limit, | |||
minLength = -1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using minLength = 0
would also work here, and avoids potential hazards due to using an "invalid" value as a sentinel. By this I mean that some code assumes a length value is >= 0, and behaves unexpectedly given a negative input.
// Too few characters are entered into the field and the field is then | ||
// "committed" (e.g. the focus leaves the field, or the form is | ||
// submitted.) | ||
fieldEl.getDOMNode().value = 'aa'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a missing fieldEl.simulate('input')
after this edit. This happens not to make a difference given the current implementation of the UI, but it should be there to match browser behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is there! I don't think I missed any?
const fieldEl = elements.name.fieldEl; | ||
|
||
// Too few characters are entered into the field and the field is then | ||
// "committed" (e.g. the focus leaves the field, or the form is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One detail I noticed when testing is that a change
event is not emitted when focus leaves the field if the current value is the same as the previously committed value. This doesn't have an impact here, but I think is useful to be aware of.
label: string; | ||
testid: string; | ||
required?: boolean; | ||
autofocus?: boolean; | ||
classes?: string; | ||
}) { | ||
const id = useId(); | ||
const [error, setError] = useState(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, although usually you want the state to be the minimal state needed to determine the error condition, and then derive the actual message during rendering. As a contrived example, if the minLength
changed after the commit, the error message should update to reflect that.
It occurred to me that here the state could probably be reduced to "has the user made an initial commit to this field"? The code below is not exactly the same logic, but does pass the tests and would probably be equally useful for users.
diff --git a/h/static/scripts/group-forms/components/CreateGroupForm.tsx b/h/static/scripts/group-forms/components/CreateGroupForm.tsx
index d85c7f2c5..7a01dbdf7 100644
--- a/h/static/scripts/group-forms/components/CreateGroupForm.tsx
+++ b/h/static/scripts/group-forms/components/CreateGroupForm.tsx
@@ -74,22 +74,23 @@ function TextField({
classes?: string;
}) {
const id = useId();
- const [error, setError] = useState('');
+
+ // True if an initial value has been committed.
+ const [hasCommitted, setCommitted] = useState(false);
const handleInput = (e: InputEvent) => {
onChangeValue((e.target as HTMLInputElement).value);
};
const handleChange = (e: Event) => {
- if ((e.target as HTMLInputElement).value.length < minLength) {
- setError(`Must be ${minLength} characters or more.`);
- }
+ setCommitted(true);
};
+ let error = '';
if (value.length > maxLength) {
- setError(`Must be ${maxLength} characters or less.`);
- } else if (value.length >= minLength) {
- setError('');
+ error = `Must be ${maxLength} characters or less.`;
+ } else if (value.length < minLength && hasCommitted) {
+ error = `Must be ${minLength} characters or more.`;
}
const InputComponent = type === 'input' ? Input : Textarea;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to work nicely, done
@@ -116,6 +116,44 @@ describe('CreateGroupForm', () => { | |||
}); | |||
}); | |||
|
|||
describe('name field', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are general tests for the TextField
component as much as the name field. I expect it will make sense to extract that into its own module soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
When testing this I noticed it is possible to create groups with all-whitespace names (eg. 5 spaces), whereas the old create group form will prevent you from doing this. |
be084a4
to
dfd4ecf
Compare
Happy to come back to this later. I think the DOM might some way to intercept the browser validation messages, preventing the default behaviour and rendering it ourselves instead? I've added a note for it to the project board: https://github.com/orgs/hypothesis/projects/142/views/1?pane=issue&itemId=72780195 Worth noting that the current project is just to do a functionally equivalent reimplementatoin of the current page and the current page doesn't have this at all: there's no frontend min or max length validation, it lets you submit the form and then (after a full page reload) renders a validation from the server. It has a character counter that turns red but only if you exceed the max length and this does not display any frontend error message or prevent submission of the form. Not that I'm against making functional improvements over the current implementation but I agree that we can revisit it later. |
I'm thinking we may want to fix this on the backend rather than on the frontend, see #8834 and #8811 (comment) |
e86004b
to
d79f8cc
Compare
dfd4ecf
to
688806f
Compare
Screencast.from.2024-07-26.17-00-07.mp4
Testing
<3
chars in it then the field goes into its error state<3
chars in it, whether the name field is currently focused or not, the field goes into its error state (if it wasn't in it already) and a validation error is shown>=3
chars if you edit it down to<3
chars it won't immediately enter its error state until focus leaves the field or you try to submit the form<=25
chars it exits its error state. If you try to submit the form with>25
chars it shows a validation error