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

NumberInput : Fix error states #1584

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

jqlio18
Copy link
Contributor

@jqlio18 jqlio18 commented Dec 12, 2022

  1. Breaking change : Change allowEmpty attribute to required. allowEmpty makes no sense, it should always be required to match base HTML input atrributes.
  2. The input was in invalid state with a red outline and the validity of the input would stay valid (only if empty). This patch fix this.

** NOTE TO DISCUSS : This bug was caused by the ambiguity between invalid and error variables. I think that we need to remove the export invalid and let it be local only. If OP wants to validate the input, we can export the pattern or change the setCustomValidity with the ref. Since this PR check the validity of the input on input and change, the setCustomValidity will set the error.

** Edit : I forgot to link the issue #1583 . -_-

…et the `error` accordingly.

2. Fix last commits and the `invalid` state border works again.
3. If the `invalid` state is set by the user, we must use `setCustomValidity` to set input validity to false.
4. `invalidText` must have a default and must not be empty. If empty, it will set the validity to true. I don't know if we should check for that.
is now binded to the input. Required, min, max
and value are reative to error states.
@jqlio18
Copy link
Contributor Author

jqlio18 commented Dec 15, 2022

Last commit is a big step forward. Value, min, max and required are reactive and will put the input in an error if changed by OP. The value is now bind to the input since svelte parse numbers internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants