julien51
commented
5 years ago Thanks for this detailed proposal.
Before weighing further I think I need to understand better.
For now, my understanding is that on CreatorLockForm we invoke validate every time a field is changed. My understanding is that you would want to add that error to the redux store immediately. Is that correct?
I'd want first to challenge that assumption. I think that this may appear to be very 'obnoxious'. I also worry that this means the all UI will go down/up every time because the 'error' banner will take some space at the top of the form?
I think what we should do is only add events to the redux store when the user hits submit. Then we can show what is wrong with their form in more details. This would also solve the current issue where nothing happens when the user hits submit on a non valid form.
I agree that just having a red background might be not quite enough and we may want to provide more info, but I am not sure that the best place to do that is at the top of the form? (at least for now I think all our fields are fairly self-describing and a user would probably know right away what is wrong?)
I also think this is how most forms work online? They show you actual errors only when submitting and have an indication of what they expect underneath it?
What do you think about that?
If we go down this route, I think things will probably be much simpler. We then can expect the user to either close each individual error, or to change the problematic values so that when they submit, we unset all errors (and/or add new ones)
cellog
Feature request Background
This challenge is currently a blocker for another issue.
For error processing in the Creator Lock Form (#797) I have been working on a PR to implement this. In the process of doing this, I have encountered (or imagined in the case of #3) these possible scenarios:
Scenario 1: User triggers multiple errors This happens when the user resets 2 form fields to empty, for example
Scenario 2: User triggers an error, and then replaces it with another error This happens when the user deletes the key expiration and replaces it by pasting in an invalid value. The "key expiration is missing" error is replaced by "key expiration is invalid"
Scenario 3: User is editing the form, and an unrelated error condition occurs which must be displayed This could happen if, for example, the user is editing and the internet goes down, removing access to the blockchain or to locksmith
Scenario 1 is handled by implementing multiple errors #978 Scenario 2 could result in 2 errors being displayed, 1 of which is invalid and is not yet handled Scenario 3 could result in the external error being cleared, because the form resets all errors when it is valid
Proposed Solution I would like to add a new action,
bulkSetErrorswhich accepts 2 arrays. The first will be an array of errors to set, and the second an array of errors to clear. The reducer will check for any errors that need to be set, and clear any that need to be cleared, leaving all others intact. usage like:This action would provide a sandboxed state change to the error list. Only errors explicitly mentioned would be set or reset.
Other solutions considered
Currently, the only error API is
setError, which is called withnullto clear all existing errors. One solution is to add aresetErroraction, which would clear an individual error.This requires the creator lock form to trigger a
setErrororresetErroraction on validation of each form field, and will generate 7 redux actions on every keystroke. This has the disadvantage of being quite slow on a mobile device and could result in dropped keystrokes.Alternatively, the error state could be provided to
CreatorLockFormand used to only trigger aresetErrororsetErrorif the error is not currently set. This has the disadvantage of tightly coupling the creator lock form to the internal error representation.Why the proposed solution is probably the best option The error set/reset actions will be triggered quite often in form editing (every keystroke) and to avoid re-render hell, we will need an efficient solution.
Possible future enhancements If the form ends up still be unresponsive, we can implement a debouncing delay of 100ms before triggering a new action. With this implementation, that will be trivial. With multiple triggers of setError/resetError it becomes quite complex to manage the state.
In addition, a possible enhancement would be to memoize existing error state and compare to new state, avoiding triggering a new
bulkSetErrorsif state has not changed. This requires passing in the error state toCreatorLockFormso is not part of this initial design, and would only be implemented if performance becomes a real issue.Conclusion Before I implement this, I'd like feedback from @julien51 and anyone else who has a strong opinion on the question.
Once this question is settled, #797 will be ready for an initial PR