-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
NEXT button on enketo forms is available even when it doesn't do anything #3399
Comments
This is also an issue if something is wrong with z-score calculation. This form is missing the z-score calculate and stops the user from progressing with no error in console.
|
This form relies on calculation of z-scores via appearance attribute:
This method of calculation has a short delay before the value is computed during which the form cannot proceed to the next page. It would be great if the 'NEXT' button was disabled if a user cannot proceed to the next page because of a missing required value. In this case, users should endeavor to compute z scores with explicit calculation which would be more responsive. |
@garethbowen I'd really like your advice on this ticket.We worked on the first issue, disabling the Next button when there are validation errors.Whats confusing is what should happen to the form when the z-score is not working or having a delay as described by Nick and Elijah respectively. |
In the above form I had to add the z-score function for it to work properly. It would then calculate the zscore values and then I could continue on. However, the form as is would show no z-score calculations and the button would be enabled but wouldn't allow you to continue forward. If we are saying it's because it's out of date I'm ok with that. To me it seems like poor behavior to show a enabled button and be stuck. |
I think what you've created is a hidden required field with no calculation to update it. I think the fix for the original issue may mean the Next button is disabled but there will still be no way to proceed with the form you have. It'd be good to investigate the behaviour and see if it's more sensible once the original bug is fixed. |
Enketo isn't reevaluating the form |
So I took a quick look at the e2e test:
Having said that, I don't think we should actually merge this change. Unless I misunderstand the breadth of the problem this ticket aims to solve, it is a QOL improvement to disable the next button until the user is actually able to progress. While this improvement is nice, it is somewhat undercut by us apparently only having input blur (or similar) as a hook to determine whether the next button is enabled, meaning you replace a next button that is sometimes enabled when it probably shouldn't be with a next button that is disabled when it probably shouldn't be (until you tab out or click away). Thoughts @garethbowen ? |
Actually @vimago interested in your opinion here as well. Is there a smarter way of doing this? |
I think you might be right. I haven't tried this solution out, but is the problem when...
I've just tried this on my local branch and it looks like the field level validation clears on change without blur (I tested a select2 field). This should fire the It looks like you're working on this so I haven't dug in and tried this out - let me know if I can help. |
This works for select2 fields, checkboxes, radios. However, it doesn't work for input fields (try creating a patient and not filling in their name). I'm guessing because Enketo is hooked into So then, I think there are three choices:
Without any input from users as to how annoying the current situation is, my vote would be for the last one. |
A fourth option is that we listen for oninput on the whole form. Then we could either 4a) re-run validation and update the Next button, or 4b) enable the Next button right away and let the validation run when they click it. My gut feeling is 4a would be too expensive to run every key press which may be why Enketo doesn't do it. I think 4b would be a reasonable compromise between correctness and performance. |
OK @garethbowen I've done 4b, and I've also done it on the submit button for consistency. Check our the PR above and have a play and see if you like it. |
Ready for AT in |
LGTM on majority of forms. However:
(The following are also on
|
@ngaruko thanks for ATing this. On 1, can you provide reproduction steps and details (ie the actual form you're seeing this on etc) so I can look at it On 2, I've noticed this as well but it's a separate issue and so we should raise it as such. On 3, I'm not clear on the problem here? If a form field isn't required then the code wouldn't treat it as such. If you are using some standard / default config that we support (it's not clear from your post) and you think that config is wrong that would like 2 warrant a separate ticket |
@ngaruko no worries on 1, I've managed to reproduce it. For the other 2 though if you want to raise issues that would be helpful |
OK I originally wrote how I was at a dead end but I took another look and I got a bit further. This is now available in the linked PR, and should have been pushed to horti (though we are currently suffering through pretty massive e2e test stability so it's not clear). This is somewhat of a compromise:
|
@ngaruko I think this is ready for review again. |
Thanks @SCdF . That one seems to be work perfect. There are still a few areas where the app could do better in forms. 2 - Maybe a caching issue, when after being prompted to make a selection, the user goes back or to a different tab, then comes to back to the form, the error message is still showing while the Next button shows enabled (see screenshot and screen recording - for a pregnancy form) 3 - Maybe a regression here, we lost the warning when we exit the form while filling it |
Closing as won't fix as recommended by @SCdF - it's low priority and non-trivial to get right. |
Observed behaviour
When validation has failed for a page, the Next > button still looks clickable:
Expected behaviour
Some visual clue should be given that we cannot proceed to the next page, e.g. button could be grey
The text was updated successfully, but these errors were encountered: