Pattern prop for TextInput to give feedback about errors #737
Conversation
* New prop named as "pattern" avaiable to handle with input expression errors and expressions which doesn't match the regex pattern; * Supressed "console.error(err);" on idyll-document/src/utils/index.js as requested on issue (now we have a feature to give user feedback); * Modified introduction.js on idyll-docs to give stylish to the new input with validation; * Updated TextInput documentation.
|
This looks great @mrviniciux! Thanks for the PR and updating the doc strings as well. I've tested locally and it looks ready to merge, my only comment is that maybe the error message should be configurable? E.g. maybe we add something like: So you can optionally customize that message. Open to suggestions for what that property should be called |
There was a problem hiding this comment.
This looks great @mrviniciux! Thanks for the PR and updating the doc strings as well. I've tested locally and it looks ready to merge, my only comment is that maybe the error message should be configurable?
E.g. maybe we add something like:
[TextInput value:x pattern:... patternFailureMessage:"Input value doesn't match pattern." /]So you can optionally customize that message. Open to suggestions for what that property should be called
Hey @mathisonian
Thanks for the feedback. I've done the changes as requested and just changed the new prop to patternMismatchMessage, hope that works.
Can you review again?
| .idyll-root input[type='text'].idyll-input-error { | ||
| border-color: red; | ||
| } | ||
|
|
There was a problem hiding this comment.
@mathisonian I'm not sure about this, the styling works on /docs page but outside not. Should we keep as is or do something global?
| @@ -122,7 +122,7 @@ Write your own equation: | |||
| [var name:"funcString" value:\`"(x) => x * Math.sin(10 / (x || 1))"\` /] | |||
| [derived name:"funcFromString" value:\`eval(funcString)\` /] | |||
|
|
|||
| [TextInput value:funcString /] | |||
| [TextInput value:funcString pattern:\`/([a-zA-Z]\\w*|\\([a-zA-Z]\\w*(,\\s*[a-zA-Z]\\w*)*\\)) =>/\` /] | |||
There was a problem hiding this comment.
pattern gonna test if value is an arrow function string
| throw new Error(patternMismatchMessage); | ||
| } | ||
|
|
||
| eval(value); //check if there's JS syntax errors |
There was a problem hiding this comment.
Can we drop this eval()? I can imagine cases where a user could reasonably be expected to input something that isn't valid JS source code.
e.g. something like
[var name:"username" value:"" /]
Enter your name: [TextInput value:username /]
Your name is [Display value:username /]
if I enter my name it will give an "unexpected identifier" JS error
|
Thanks for the updates @mrviniciux, things are looking good! I left a comment related to where to put styles, and added one question about the eval. I'm happy to merge once those are addressed. |
* Add highlight error classes for idyll-theme styles and for idyll-docs demo component; * Removed eval error handling on text-input
Any time @mathisonian ! And I pushed a new commit with all requested alterations you mentioned. Hope that works |
|
Looks great, thanks @mrviniciux! Merging this and will have it out in the next release on npm |
|
@mathisonian I think you closed the PR, is that right? And also thank you again for your fast feedback |
|
D'oh, thanks for catching that 🙈. Actually merged now. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
New prop named as "pattern" avaiable to handle with input expression errors and expressions which doesn't match the regex pattern;
Supressed "console.error(err);" on idyll-document/src/utils/index.js as requested on issue (now we have a feature to give user feedback);
Modified introduction.js on idyll-docs to give stylish to the new input with validation;
Updated TextInput documentation.
What is the current behavior? (You can also link to an open issue here)
When the input was part of an evaluated expression and that expression was invalid an error occurs and also throwed on dev console leading to crash. See issue here: #675
What is the new behavior (if this is a feature change)?
An error message is displayed within the input, so the user can have some feedback. Also the error on console is now suppressed.
Other information: