-
-
Notifications
You must be signed in to change notification settings - Fork 603
Description
Hi there!
We currently have a requirement for a custom cropped image field. We have allowed multiple crops on this field (for different aspect ratios, etc) but are having to do this across different arrays within the attribute, eg -
[CroppedImageFieldSettings(AspectRatios = new double[] { 16d / 9d , 1d / 1d }, Crops = new string[] { "Default", "Second Crop" })]
This doesn't feel great as the aspect ratios and any other settings need to line up with the indexes of the differing crops. It's a little difficult to tell what's going on and, as we potentially add more settings, it becomes easier to make a mistake when defining this field.
The solution would be to allow multiple field settings attributes on a custom field. I've looked into the source code and it looks like this was almost supported at one point? I only have to make a few changes to this method for it to work - change the GetCustomAttribute call to GetCustomAttributes then simply iterate through it instead of doing the null check. Something like
public static IDictionary<string, object> GetFieldSettings(PropertyInfo prop)
{
var settings = new Dictionary<string, object>();
// Get optional settings
var settingsAttrs = prop.GetCustomAttributes<FieldSettingsAttribute>();
foreach (var settingsAttr in settingsAttrs)
{
foreach (var setting in settingsAttr.GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.DeclaredOnly))
{
if (settings.TryGetValue(setting.Name, out var existing))
{
if (existing is not IList)
{
existing = new ArrayList
{
existing
};
settings[setting.Name] = existing;
}
((IList)existing).Add(setting.GetValue(settingsAttr));
}
else
{
settings[setting.Name] = setting.GetValue(settingsAttr);
}
}
}
return settings;
}Do you foresee any problems with this approach, or indeed with the concept of multiple attributes here? This only appears to cause breaking changes specifically on custom attributes that already have the AllowMultiple property set to true and shouldn't affect any of Piranha's default fields. Let me know if this sounds good and I can put together a PR. 🙂