Changes to allow localisation/overwriting strings #15
Conversation
…ute description as secondary source of label.
… with resources from a specified assembly, allowing for localisation. Original functionality should be preserved for English.
…overwritten to allow for localisation. Current functionality and strings for English are preserved.
jamiehumphries
changed the title
| public string ErrorMessageIfMissing | ||
| { | ||
| get => GetResourceValue(_errorMessageIfMissing); | ||
| private set => _errorMessageIfMissing = value; |
There was a problem hiding this comment.
As a general thing, having a property that returns a different value to what is set makes me extremely uncomfortable. You could just set the private variable in the constructor instead.
There was a problem hiding this comment.
Actually, does this rely on the base class behaviour of returning the resource key if it's not found to pass through errorMessageIfMissing if it's actually some text?
I think this either needs explicitly named fields for each or much clearer naming of the existing shared field.
There was a problem hiding this comment.
Hmm, the more I look at this, the more I think it would make sense to just create a new GovUkDataBindingLocalisableMandatoryDecimalErrorTextAttribute in parallel to this one.
It would mean a large, but straightforward search and replace in the SEA codebase but I think it would be much clearer.
There was a problem hiding this comment.
I think I've managed to do this separation, but would like some feedback on how I've implemented it. It's actually a smaller change in SEA as these are only used in a few places.
GovUkDesignSystem/Attributes/DataBinding/GovUkDataBindingMandatoryDecimalErrorTextAttribute.cs
Outdated
Show resolved
Hide resolved
GovUkDesignSystem/Attributes/GovUkRadioCheckboxLabelTextAttribute.cs
Outdated
Show resolved
Hide resolved
GovUkDesignSystem/GovUkDesignSystemComponents/FooterViewModel.cs
Outdated
Show resolved
Hide resolved
| @@ -38,7 +38,7 @@ public static class ErrorSummaryHtmlGenerator | |||
| { | |||
| Title = new ErrorSummaryTitle | |||
| { | |||
| Text = "There is a problem" | |||
| Text = errorSummaryTitle | |||
There was a problem hiding this comment.
Arguably the translation for this (and things like the copyright notice) should actually be part of this library. However it doesn't seem worth the effort right now for this minimal amount of text.
…helper extension arguments.Radio checkbox using Single instead of First. Simplified footer property names.
…eby "Display" attributes can be used in addition to the custom GovDesignLibrary attributes when defining which label to show for radio buttons and checkboxes
| public string ErrorMessageIfMissing { get; private set; } | ||
| public virtual string ErrorMessageIfMissing { get; } | ||
|
|
||
| public virtual string IsWholeNumberErrorMessage => ""; |
There was a problem hiding this comment.
I'm not keen on adding these properties to these classes that will never use them, but I don't think it's worth the effort to change it at the moment.
I'm not sure exactly what I'd do differently to simplify things, but perhaps the model binder could look for two different attribute types. I might even go further and create two different model binders (one localisable) with a shared base.
Resubmission of #16 Changes to allow localisation/overwriting strings
Relates to PC-599 for beis-simple-energy-advice-beta.