Skip to content

Sane textfield #29

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

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

Sane textfield #29

wants to merge 3 commits into from

Conversation

sourcegr
Copy link

@sourcegr sourcegr commented Jun 30, 2020

Hello @vikignt !

The Textfield component was kind of buggy, at least in our workflow.

  • When the error prop was present, it would show no matter what.
  • When the error prop was present, even when the required field was set and the input was filled, the error would still persist.

With this PR, the following things happen, hopefully setting some things straight and making it more a component than a display.

  • added avalidator callback to check the value of the component against, and set theinvalid flag on it when false is returned.
  • Theerror is not displayed, unless the component value is actually invalid (invalid is - for example - an empty string when the required is set, or a value that fails the validator callback.)
  • The error is not displayed, unless a value is provided and the value is invalid, preventing the error message to display all the time.

We also believe that the default value for messagePersist should change to true - since we always - always - have to fill this up. I would like to hear what other users think on this.

@sourcegr
Copy link
Author

You may want to keep it simple, and this is fully understood.
The truth is that most of the tooling provided by this PR can be accomplished from the outside.

But since on 99% of the occasions a check is required in order to validate the contents for a field, it feels a little overhead to do it again and again all over, setting and unsetting error messages in the code.

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

Successfully merging this pull request may close these issues.

1 participant