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

[Form Validation] Add form validation rule: condition #988

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

vibridi
Copy link

@vibridi vibridi commented Sep 1, 2019

Description

This PR adds the condition validation rule. It allows to apply an arbitrary rule to a field value depending on another field value.

Test Case

After: JSFiddle

Closes

#985

@lubber-de lubber-de added state/awaiting-reviews Pull requests which are waiting for reviews type/feat Any feature requests or improvements lang/javascript Anything involving JavaScript state/awaiting-docs Pull requests which need doc changes/additions labels Sep 1, 2019
@y0hami y0hami added this to the 2.8.0 milestone Sep 1, 2019
@lubber-de lubber-de added the state/awaiting-response Issues or pull requests waiting for a response label Sep 7, 2019
@vibridi vibridi force-pushed the FUI-985/form-field-value-rule branch from dc402cc to 4159e1f Compare September 9, 2019 11:46
@y0hami y0hami removed the state/awaiting-response Issues or pull requests waiting for a response label Sep 9, 2019
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

Please adjust . includes() to make it work in ie11

src/definitions/behaviors/form.js Outdated Show resolved Hide resolved
Add the 'condition' validation rule. It allows
form field values to be validated when another field is
set to a certain value.

Closes fomantic#985
lubber-de
lubber-de previously approved these changes Sep 9, 2019
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM 🙂 👍

prudho
prudho previously approved these changes Sep 12, 2019
Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

LGTM

ColinFrick
ColinFrick previously approved these changes Oct 2, 2019
Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1823,6 +1824,58 @@ $.fn.form.settings = {
;
},

// applies rule if another field's value satisfies a condition
condition: function(value, ancillary, $module) {
Copy link
Contributor

@exoego exoego Oct 2, 2019

Choose a reason for hiding this comment

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

I have a little difficulty to understand the new syntax...

My understanding is condition[FIELD_NAME, RULE1, RULE2, ... RULEn].
Am I on the right track?
If so, I feel it is a bit strange to treat FIELD_NAME and RULEs in the flat way.

I suggest a JSON-like syntax like { FIELD_NAME: [RULE], FIELD_NAME2: [RULE1, RULE2] }.
This syntax distinguishes FIELD_NAME and RULEs.
This will also can cover use-case to have conditions on multiple fields.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, the current way will be too confusing for users and doesn't allow for expansion of the rule. I think your suggestion of making it JSON like will be our best shot to making it easy to use and easy to improve in the future.

Copy link
Author

Choose a reason for hiding this comment

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

@exoego @hammy2899 Hello guys, based on what the author of this issue (#985) writes:

A built-in rule to validate a form field based on the value from another field of the same form.

I intended to lay out the parameters as follows, basically using the syntax proposed by the issue author:
condition[FIELD_NAME, VALUE_TO_CHECK_AGAINST, RULE_TO_APPLY]
which means: "if THAT_FIELD's value is VALUE, then apply RULE to this field".

Also I intended to set a hard limit on what RULE_TO_APPLY can be, namely one single existing rule, on purpose. I was looking to not risk having recursive rules, or excessively complicated config.

With that said I'm absolutely open to improve this. If you think JSON-like syntax is the way to go, I'll change accordingly 👍

Copy link
Member

Choose a reason for hiding this comment

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

@hammy2899 @exoego What are we going to do with this now?

Copy link
Member

Choose a reason for hiding this comment

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

I think making this JSON like is still the best option we have especially for future support.

Copy link
Author

Choose a reason for hiding this comment

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

@hammy2899 please can you set out a small spec of how you envision this being JSON-like, then I'll try to take it from there. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I have a little difficulty to understand the new syntax...

My understanding is condition[FIELD_NAME, RULE1, RULE2, ... RULEn].
Am I on the right track?
If so, I feel it is a bit strange to treat FIELD_NAME and RULEs in the flat way.

I suggest a JSON-like syntax like { FIELD_NAME: [RULE], FIELD_NAME2: [RULE1, RULE2] }.
This syntax distinguishes FIELD_NAME and RULEs.
This will also can cover use-case to have conditions on multiple fields.

Like how @exoego said

Copy link
Member

Choose a reason for hiding this comment

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

@vibridi Are you still interested in continuing to adjust the PR ?

Copy link
Author

Choose a reason for hiding this comment

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

@lubber-de I'm sorry, I don't think I will be able to work on this at this time. Feel free to close or take over. Thanks!

@y0hami y0hami added the state/on-hold Issues and pull requests which are on hold for any reason label Nov 5, 2019
@y0hami y0hami modified the milestones: 2.8.1, 2.8.x Nov 13, 2019
@lubber-de lubber-de requested a review from ko2in June 9, 2020 12:00
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Dec 24, 2020
@lubber-de lubber-de marked this pull request as draft May 16, 2021 13:46
@lubber-de lubber-de removed this from the 2.8.x milestone Sep 6, 2021
@lubber-de lubber-de dismissed stale reviews from ColinFrick, prudho, and themself via ed995fc June 8, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript state/awaiting-docs Pull requests which need doc changes/additions state/on-hold Issues and pull requests which are on hold for any reason type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants