Skip to content

Conversation

@bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Jul 14, 2022

image

image

image

@eshanrnh
Copy link
Contributor

Thanks for the PR, @bjarnef 🙌 We will review this soon 🙂

@eshanrnh eshanrnh requested a review from AndyButland July 14, 2022 10:17
@bjarnef
Copy link
Contributor Author

bjarnef commented Jul 14, 2022

I think @AndyButland may have some input and the "CompareField" setting currently just contain alias of the other field.
Not sure how we can access the FormId or get current form within a field type context.

It may need to field to specificy validation message as there are tree validation types:

  • Mandatory
  • Email (regex)
  • EqualTo

Email (regex) could be optional. In other scenarios in may be Phone instead or another custom regex.

If including https://cdnjs.cloudflare.com/ajax/libs/jquery-validate/1.19.5/additional-methods.js there's also a NotEqualTo method.

I think in "Compare" filed type could be useful in a future release of Umbraco Forms.

: string.Empty;
}

private static Field? GetPostField(Form form, string key) => form.AllFields.SingleOrDefault(f => f.Alias == key);
Copy link
Contributor Author

@bjarnef bjarnef Jul 14, 2022

Choose a reason for hiding this comment

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

@AndyButland I noticed these methods in other examples, but maybe Forms should include these extension/helper methods?

E.g.

GetField(Form form, string key)
GetFieldValue(Form form, HttpContext context, string key)

Copy link
Contributor

@AndyButland AndyButland left a comment

Choose a reason for hiding this comment

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

This is a nice solution, but I think it might be better as a blog post we link to (which I'd be happy to do so from the docs) rather than going in as official documentation. There's a few reasons for this:

  • There's a hard-coded reference to a form ID. Whilst that might work for a particular scenario, it's not a very general approach that we can really recommend people do.
  • You've got client-side validation working with jquery which is great, but not sure if it also works with aspnet-client-validation? I think it would need to given people are often moving away from jquery now for something more lightweight.
  • The compare field is missing most of the settings available on the text field. And it would be an overhead in code and in docs to maintain this.

This is just an idea, but I think the best way to support this in Forms may be something you configure at the level of the form rather than the field. So like you can add workflows for a form, perhaps we have some sort of means to add "form validation rules". Validation on single fields would be set as is, but rules that span fields across the form would be set here. So you could have "compare email = email", "end date > start date" etc. without having to have "compare" versions of many of the field types.

We would then need to have client side and server side validation of these rules as the form and/or page is submitted.

@bjarnef
Copy link
Contributor Author

bjarnef commented Jul 18, 2022

@AndyButland regarding the hard-coded reference to a form ID. Yes, I don't want it to be hard-coded, but I didn't found a way to get current form reference in field types (like in form template itself). Do field types have any access to the form context, e.g. get current form or just a Guid reference to the form?

Currently it is using equal-to which is available in jQuery validation, but may work without jQuery if the client side validation works using same attribute. The other field types use similar attributes like data-val-required , data-val-regex and data-val-regex-pattern. Not sure how it works without jQuery and jQuery validation? Does it just ignore these attributes or does it by coincidence have client side validations in aspnet-client-validation using same attributes?

Which specific settings are missing for Compare field? It is basically a copy / extended version of FieldType.Textfield.cshtml (but the textfield type may contain more server side logic, which isn't public available).

I was thinking about compare rules as well, but it isn't a concept in Forms at the moment, so this was an attempt for now with the current features.

@AndyButland
Copy link
Contributor

AndyButland commented Jul 18, 2022

Do field types have any access to the form context, e.g. get current form or just a Guid reference to the form?

From within the partial view, I don't believe so - they just have access to the FieldViewModel.

Not sure how it works without jQuery and jQuery validation? Does it just ignore these attributes or does it by coincidence have client side validations in aspnet-client-validation using same attributes?

I suspect the new library tries to follow the same conventions, as it's intended as a replacement for using jquery. But it would need testing to confirm.

Which specific settings are missing for Compare field? It is basically a copy / extended version of FieldType.Textfield.cshtm

I meant the settings on the field type definition - so autocomplete attribute, field type, placeholder etc.

@bjarnef
Copy link
Contributor Author

bjarnef commented Jul 18, 2022

@AndyButland okay, I am not sure how FieldViewModel is populated with data, but I guess since it has is populate with e.g. Caption and Alias of the field in the current form, this logic already have access to the form. So could all field types e.g. have a FormId property. Then it would be easier to find the form without hardcoding the Guid form reference.

Not sure if aspnet-client-validation is based on this, but in that case there's a equalto (compare) method as well:
https://github.com/haacked/aspnet-client-validation/blob/3d754e9613e84f689dee3fb77fbcbf08a84f731d/dist/aspnet-validation.js#L530

Yes, you're are right .. I added the settings to the view, but not to the field type. I tried originally to inherit from the Textfield provider, but this override this existing Textfield (Short answer) like this https://our.umbraco.com/documentation/Add-ons/UmbracoForms/Developer/Extending/Adding-a-Type#overriding-default-providers-in-umbraco-forms
But it was the intention to have same setting + addition settings.

Can we extend/inherit a field type from an existing field type e.g. Textfield instead of Umbraco.Forms.Core.FieldType? But then Umbraco Forms could e.g. later add new settings to Textfield, which may also require additional updates in the view of the custom field type. So it may be safer to include the properties as well in the custom field type.

@bjarnef
Copy link
Contributor Author

bjarnef commented Jul 18, 2022

@AndyButland I tried removing jQuery + jQuery validation and instead included @Html.RenderUmbracoFormDependencies() mentioned here: https://our.umbraco.com/documentation/Add-ons/umbracoforms/developer/Prepping-Frontend/

The compare client side validation doesn't quite seem to work, but the JS included /App_Plugins/UmbracoForms/Assets/aspnet-client-validation/dist/aspnet-validation.min.js does contain a equalTo provider method to compare fields.

But while I was using data-val-equalto-other="@field?.Id" in this example, it seems it should be data-val-equalto-other="*.@field?.Id" using aspnet-client-validation.

However in my tests jQuery validation re-validated when changing values in both fields, where aspnet-client-validation only seems to re-validate when updating the "email confirm" field (so when updating the other "email" field to an identical value, it still shows the field as invalid (not equal to) until updating "email confirm" field.

Copy link
Contributor

@sophie-neale sophie-neale left a comment

Choose a reason for hiding this comment

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

Spotted a little duplication 👀

Co-authored-by: Sophie <35264991+sophie-neale@users.noreply.github.com>
@sofietoft
Copy link
Contributor

Hi all 👋

What is the status of this PR?
It's been a while since we've seen any activity - @AndyButland suggests this would be better as a blog post, and we'd definitely be up for linking to it.
Is that something you'd be up for doing @bjarnef ? 😄
It also looks like a related issue was solved 💪

@bjarnef
Copy link
Contributor Author

bjarnef commented Dec 6, 2022

@sofietoft I don't really write blog posts, but even anyone shared that I am not sure it is discoverable as the documentation. I think it could make sense to show a few examples / code snippets to extend CMS, Forms etc.

I think it is a typical example to add custom validation and often it probably depends on another form field like in this use-case.

@sofietoft
Copy link
Contributor

I completely understand @bjarnef ! 💪
We wanna have all the relevant examples in the docs, of course!

Let me just have another chat with Andy about it, and we'll figure out what we should do with it.

@sofietoft
Copy link
Contributor

Hi @bjarnef !

I've had a chat with @AndyButland about this, and we've agreed to not add this sample to our docs.
We do not feel that this sample represents the general best practice that we want the Umbraco docs to represent.

We do agree, though, that it's a sample that would definitely be valuable to other people. The optimal solution here would be to have the option to add this sample in the docs and then marking it as "community content".
Unfortunately, we do not have a good way of doing something like that at the moment 😞 I'll be having a chat with the DevRel team about it, as I think it would be really valuable having a place for these community contributions that don't quite fit our docs but still has a lot of value to them!

I'll be closing this PR for now though.
I hope you understand! Please do let me know if you have further questions 🙌

And thanks again for putting in the work here - we really appreciate it!

@sofietoft sofietoft closed this Dec 7, 2022
@bjarnef bjarnef deleted the patch-26 branch May 30, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants