Custom Validation Attribute & checkbox Divider functionality #17
Conversation
GovUkDesignSystem/Attributes/ValidationAttributes/GovUkValidateCustom.cs
Outdated
Show resolved
Hide resolved
GovUkDesignSystem/Attributes/ValidationAttributes/GovUkValidateCustom.cs
Outdated
Show resolved
Hide resolved
| if (propertyInfo is null) | ||
| { | ||
| throw new ArgumentException( | ||
| $"'{CustomValidationPropertyName}' must be a boolean property in the model the attribute is included in"); |
There was a problem hiding this comment.
This is not actually what you're checking above.
Maybe you also want to check that it's a read/get-only property?
There was a problem hiding this comment.
I didn't realise you could check readonly!
I think this check here is something I missed from reusing RequiredIf
| @@ -99,6 +101,7 @@ public static async Task<IHtmlContent> GovUkCheckboxesFor<TModel, TEnum>( | |||
| classOptions, | |||
| conditionalOptions, | |||
| idPrefix, | |||
| dividerOptions, | |||
There was a problem hiding this comment.
The interface for this is a bit weird on the other end: you pass a dictionary of options vs the divider word that should come above the option. I don't know if there's a way to make that naturally more intuitive, but I think the library should document how you're supposed to use it.
There was a problem hiding this comment.
I'm not quite sure how to make the .Select in the CheckboxesHtmlGenerator work in the more intuitive way, given that the behaviour is embedded all the way into the partial for "Item" in general.
I've updated the comment in ItemViewModel to be more descriptive about its behaviour, but I'm not sure if that's an appropriate place, given that it being "before" is a product of Item.cshtml, not the view model.
There was a problem hiding this comment.
I think this is going to just be a bit complicated, yes. That's why it's important we document it, so someone doesn't have to dig through the (complicated) library code to figure out what it's supposed to do.
If you add a summary comment to this GovUkCheckboxesFor method then it should show up when you use the method and hover over it. I think describing what parameters are expected to do there would be good enough.
… name and error checks
GovUkDesignSystem/Attributes/ValidationAttributes/GovUkHasCustomValidator.cs
Outdated
Show resolved
Hide resolved
GovUkDesignSystem/Attributes/ValidationAttributes/GovUkHasCustomValidator.cs
Outdated
Show resolved
Hide resolved
GovUkDesignSystem/Attributes/ValidationAttributes/GovUkHasCustomValidator.cs
Outdated
Show resolved
Hide resolved
GovUkDesignSystem/Attributes/ValidationAttributes/GovUkHasCustomValidator.cs
Outdated
Show resolved
Hide resolved
| if (propertyInfo.CanWrite == false) | ||
| throw new ArgumentException($"'{CustomerValidatorPropertyName}' must be a read only property"); | ||
|
|
||
| var isValid = (bool)propertyInfo.GetValue(validationContext.ObjectInstance, null)!; |
There was a problem hiding this comment.
We should probably not cast here unless we really need to: what would it mean if we got null back?
There was a problem hiding this comment.
GetValue returns an object type, so casting it to bool allows us to do the check below
There was a problem hiding this comment.
We shouldn't get null back also, because we've checked it's a boolean property above, not a nullable boolean.
There was a problem hiding this comment.
Expect we'll still need the (bool), but do we still want the ! if the check is the right way round above?
I don't think you need the second argument here either.
There was a problem hiding this comment.
I've removed the second argument, but it still doesn't like the potential null, even with the correct way around check above. I'm not entirely sure why that is
GovUkDesignSystem/Attributes/ValidationAttributes/GovUkHasCustomValidator.cs
Outdated
Show resolved
Hide resolved
GovUkDesignSystem/Attributes/ValidationAttributes/GovUkHasCustomValidator.cs
Outdated
Show resolved
Hide resolved
| if (propertyInfo.CanWrite == false) | ||
| throw new ArgumentException($"'{CustomerValidatorPropertyName}' must be a read only property"); | ||
|
|
||
| var isValid = (bool)propertyInfo.GetValue(validationContext.ObjectInstance, null)!; |
There was a problem hiding this comment.
Expect we'll still need the (bool), but do we still want the ! if the check is the right way round above?
I don't think you need the second argument here either.
|
I'm sure I made a few of the changes you've mentioned above locally before I pushed last time? Especially the typeof(bool) one. Let me see what's happened, I think some of them haven't come through |
|
It looks like my last push didn't go through completely somehow? I've pushed again now :) |
This PR is to my localisation branch, to make it easy to see the changes I've implemented for PC-770 on FWTS.