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

[ErrorProp]: Implements an error prop (fixes #693) #852

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aFarkas
Copy link

@aFarkas aFarkas commented Sep 16, 2022

@lookfirst It took me a little bit longer but here is now the PR.

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2022

CLA assistant check
All committers have signed the CLA.

@lookfirst
Copy link
Owner

@aFarkas Nice work! A couple things, I'd like resolved before merging.

  1. centralize the common logic code to DRY things. for example, the takeText can be a function defined in util.
  2. add unit tests for the TextField that tests these error conditions. those tests should then be copied to other components as well, but i can help with that if you need (or you can just do it).

Thanks again for your time and contribution, super appreciate it.

@aFarkas
Copy link
Author

aFarkas commented Sep 19, 2022

@lookfirst
I can earliest start working on it on 1. - 3. oct.

@lookfirst
Copy link
Owner

Thanks for the update @aFarkas! No rush... just when you have time. It isn't like this is paid open source... if someone else sees this and wants to contribute, feel free as well! =) This is a team effort here.

@lookfirst
Copy link
Owner

Pinging you @aFarkas

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.

3 participants