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

Basic validation #207

Merged
merged 2 commits into from
Mar 4, 2024
Merged

Basic validation #207

merged 2 commits into from
Mar 4, 2024

Conversation

VojtechLunak
Copy link
Collaborator

@VojtechLunak VojtechLunak commented Apr 17, 2023

Implements also #153.

InputAnswer

  • PATTERN attribute introduced to JSONLD question object ~ Regex expression
  • HAS_VALIDATION_MESSAGE contains the message that shows when there is invalid input
  • only "text" input (which is default) is checked for pattern

DefaultInput

  • render react-bootstrap component in isInvalid state and show error message

Further analysis

@blcham


ValidatorFactory

  • used in Question component in _handleChange method
  • checks if answer is required by looking for Constants.REQUIRES_ANSWER
    • if answer is required, than it just looks if answer has value and sets Constants.HAS_VALID_ANSWER to true, else it sets it to false and sets Constants.HAS_VALIDATION_MESSAGE as well
    • if answer is not required, it sets Constants.HAS_VALID_ANSWER to true by default

DefaultInput

  • react-bootstrap uses underlying HTML input's restrictions to define required field, patterns, min, max values, input lengths and so on
  • these restrictions can be checked at different times (onChange, onSubmit, onBlur)
    • by default, HTML input's validity is checked onSubmit (when the submit button of the form is clicked)

@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for s-forms-kbss ready!

Name Link
🔨 Latest commit 6a690b7
🔍 Latest deploy log https://app.netlify.com/sites/s-forms-kbss/deploys/65e5ed62d639ff000866baa9
😎 Deploy Preview https://deploy-preview-207--s-forms-kbss.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@VojtechLunak
Copy link
Collaborator Author

VojtechLunak commented Apr 24, 2023

@blcham test

There are two cases of validation to consider.
One way is onChange for each input.
Other way is to save onSubmit.

And then there is the option of combining these two.

@VojtechLunak
Copy link
Collaborator Author

VojtechLunak commented Jun 21, 2023

With the ValidatorFactories, there might be a problem when chaining different data rules and requirements.

Imagine these situations:

  • required answer + wanted pattern matching
    • currently, there is no mechanism to chain two validators to one Question (setting Validator code); we can only validate required answer or check answer's pattern
  • not required answer + when answer is present, check it for pattern
    • currently, this is doable by applying the MatchesPatternValidator (code)

Plus there are code duplicities in ValidatorFactory, I might propose cleaner solution later.

Abstract proposal for chaining multiple rules:

  • correct data definition in the JSONLD object
  • attaching multiple validations to one question and validate onChange

Checking data types is done by correct data type definition in JSONLD when generating form, and then DefaultInput renders the form field with corresponding React-Bootstrap component, that already has data type checks (number, date, ...).

For example:
Answer is required and has a pattern.

Scan flag 'required' and 'pattern' from JSONLD data structure.

Then onChange do validation for each flag via Validator.
For this solution to work, there must be some rule for specifying the most important flags for setting their execution order.

@blcham blcham force-pushed the basic-validation branch 2 times, most recently from 69dff40 to 8ccb2e4 Compare July 12, 2023 14:28
@blcham
Copy link
Collaborator

blcham commented Jul 12, 2023

@VojtechLunak I rebased your implementation of validators but it fails in tests

@LaChope LaChope linked an issue Feb 8, 2024 that may be closed by this pull request
3 tasks
@blcham
Copy link
Collaborator

blcham commented Feb 8, 2024

  • it is essential to check if the required values are working and how
  • try to draw how validation works
  • make simple test cases in demo

Less important:

  • validation by regexp could be evaluated immediately as it is now, but not being that aggressive (by showing red text, but rather some kind of warning). Only on the input field focus lost it should show an error (red text).

@LaChope LaChope requested a review from blcham February 16, 2024 16:58
src/constants/Constants.js Outdated Show resolved Hide resolved
src/constants/Constants.js Outdated Show resolved Hide resolved
src/model/ValidatorFactory.js Outdated Show resolved Hide resolved
src/model/ValidatorFactory.js Outdated Show resolved Hide resolved
src/stories/assets/form/form2.json Outdated Show resolved Hide resolved
@blcham
Copy link
Collaborator

blcham commented Feb 23, 2024


  1. Proposal

We need only following properties:


  1. Proposal:

We need only following properties:


@LaChope we agreed on 2. Proposal

@LaChope LaChope requested a review from blcham February 26, 2024 12:35
Copy link
Collaborator

@blcham blcham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments. Moreover, see my comments from testing:
image

src/components/answer/InputAnswer.jsx Outdated Show resolved Hide resolved
src/model/ValidatorFactory.js Outdated Show resolved Hide resolved
src/model/ValidatorFactory.js Outdated Show resolved Hide resolved
src/styles/s-forms.css Show resolved Hide resolved
src/model/ValidatorFactory.js Outdated Show resolved Hide resolved
@LaChope
Copy link
Collaborator

LaChope commented Feb 27, 2024

See my comments. Moreover, see my comments from testing: image

@blcham These are the messages from the pattern validator. I am not sure how to make better messages, it used to be the regex displayed but IMO it does not make sense to show it to the end user. This message is the same one used in Google Forms but I agree that it could be more explicit, do you have any recommendations?

My argument is that if we say something like "The pattern is not matched" or something similar, the user will expect to have the pattern, but regex will not be understandable. "Enter a valid answer" to me is not specific on purpose and relies on the user to understand the question.

@LaChope
Copy link
Collaborator

LaChope commented Feb 27, 2024

Validation message should support custom message, if no custom message, show regex

@LaChope LaChope requested a review from blcham February 27, 2024 11:49
Copy link
Collaborator

@blcham blcham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider my comments. Moreover, I have a suggestion to meet over !important.

src/components/answer/InputAnswer.jsx Outdated Show resolved Hide resolved
src/constants/Constants.js Outdated Show resolved Hide resolved
@LaChope
Copy link
Collaborator

LaChope commented Mar 1, 2024

Consider my comments. Moreover, I have a suggestion to meet over !important.

I was able to fix it by simply adding the properties to :focus (see b77c564).

@LaChope LaChope requested a review from blcham March 1, 2024 08:37
src/components/answer/InputAnswer.jsx Outdated Show resolved Hide resolved
src/constants/Constants.js Outdated Show resolved Hide resolved
src/model/ValidatorFactory.js Show resolved Hide resolved
@LaChope LaChope requested a review from blcham March 4, 2024 09:18
src/constants/Constants.js Outdated Show resolved Hide resolved
@blcham blcham self-requested a review March 4, 2024 11:15
props.validation = "error";
if (
question[Constants.USED_ONLY_FOR_COMPLETENESS] === true &&
question[Constants.HAS_VALIDATION_TYPE] ===
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe should not be here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yes, sorry I was confused

@LaChope LaChope requested a review from blcham March 4, 2024 12:11
@kbss-cvut kbss-cvut deleted a comment from LaChope Mar 4, 2024
@kbss-cvut kbss-cvut deleted a comment from LaChope Mar 4, 2024
@kbss-cvut kbss-cvut deleted a comment from LaChope Mar 4, 2024
src/stories/assets/form/form1.json Outdated Show resolved Hide resolved
@LaChope LaChope merged commit 43b8e25 into master Mar 4, 2024
7 checks passed
@LaChope LaChope deleted the basic-validation branch March 21, 2024 09:45
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.

Handle basic validation
3 participants