-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Hotfix: preload values to exisitng field types, fix fieldOrder #501
Conversation
@@ -98,7 +96,7 @@ const FormBuilder = function(opts, element) { | |||
} | |||
let icon = custom.icon || controlClass.icon(type); | |||
let label = custom.label || controlClass.label(type); | |||
let iconClassName = !icon ? custom.iconClassName || `icon-${type}` : ''; | |||
let iconClassName = !icon ? custom.iconClassName || `icon-${type.replace(/-[\d]{4}$/, '')}` : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinchappell is this icon-${type.replace(/-[\d]{4}$/, '')}
there to handle the custom fields?
If so a better way to write this section may be:
let iconClassName = '';
if (!icon) {
icon = custom ? custom.iconClassName || `icon-${custom.type}` : `icon-${type}`;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on the custom type but since since the custom type key is randomly generated each time, even if there was an icon of say 'autocomplete-2847' those last 4 digits would change on each initialization. The regex removes hyphen and 4 digits which should only be generated by custom fields. If the custom field is actually an extended base type like in the autocomplete example, its possible the autocomplete icon still wants to be used. To make that happen we strip the '-2847' leaving 'icon-autocomplete'. If they don't want the autocomplete icon they can always explicitly define an icon or iconClassName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I guess either way works. My implementation above removes ignores the '-1234' by just using the original type defined in opts.fields
. So
var fields [
{
type => 'text',
...
}
];
... would then output icon-text
for that custom field.
Either way should have the same outcome so up to you which you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok. We can always change it if it becomes an issue.
}); | ||
} | ||
|
||
// remove disableFields | ||
if (opts.disableFields) { | ||
controls = controls.filter(type => opts.disableFields.indexOf(type) == -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go either before for
loop above, or filter out controlList / allControls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to orderFields in helpers
Can now preload values to existing field types set via the
fields
option. Example:resolves #377, resolves #275