Add feature: MultiCheckbox (checkboxes with multiple choices). #42
Conversation
| @@ -29,6 +29,7 @@ | |||
| 'select' => \Givebutter\LaravelCustomFields\FieldTypes\SelectFieldType::class, | |||
| 'textarea' => \Givebutter\LaravelCustomFields\FieldTypes\TextareaFieldType::class, | |||
| 'text' => \Givebutter\LaravelCustomFields\FieldTypes\TextFieldType::class, | |||
| 'multicheckbox' => \Givebutter\LaravelCustomFields\FieldTypes\MultiCheckboxFieldType::class, | |||
There was a problem hiding this comment.
Rename "multicheckbox" to "multiselect". Checkbox is overly-specific and the input doesn't always have to be a checkbox. (i.e. <select multiple>)
| $this->validationPrefix.$this->field->id => array_filter([ | ||
| $this->requiredRule($attributes['required']), | ||
| 'array', | ||
| $attributes['required'] ? 'min:1' : null | ||
| ]), |
There was a problem hiding this comment.
The required rule implies that there will be at least 1 value, so you can drop the min:1 ternary and the array_filter wrapper.
| 'required', | ||
| 'string', | ||
| 'max:255', | ||
| Rule::in($this->field->answers), |
There was a problem hiding this comment.
Let's also add distinct here just to make sure there are no duplicates.
| public function getValue(): mixed | ||
| { | ||
| return $this->formatValue( | ||
| $this->response->getAttribute($this::VALUE_FIELD) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Can drop this as it doesn't modify the behavior of the parent class.
| public function formatValue(mixed $value): mixed | ||
| { | ||
| if (! is_array($value)) { | ||
| return [$value]; | ||
| } | ||
|
|
||
| return $value; | ||
| } |
There was a problem hiding this comment.
Can drop this too. I think it's overly cautious to not trust the data in our DB to not be an array as that's the format we require in the validation.
| $answers = $this->response->field->answers; | ||
| $values = $this->response->value; | ||
| $list = []; | ||
|
|
||
| if(! is_array($values)) { | ||
| $values = [$values]; | ||
| } | ||
|
|
||
| foreach ($values as $value) { | ||
| if (isset($answers[$value])) { | ||
| $list[] = $answers[$value]; | ||
| } | ||
| } | ||
|
|
||
| return implode(', ', $list); |
There was a problem hiding this comment.
I think all we need is a guard clause for null because as I said above it'll either be an array or null. We also shouldn't need to check isset on the available answers. Try this:
if (is_null($this->getValue())) {
return '';
}
return Arr::join($this->getValue(), ', ');
|
Hi Clint, thanks for your reply and sorry for my late answer. I'm happy to make these changes, though I'm interested in your internal version as well. If it's better I don't see why my pull request should be added back into the package :-) I'm on holiday the next weeks, but I'll be sure to make these adjustments when I am back. Thanks again. |
|
The versions are very close other than the few comments I left, so I was trying to be nice and allow you to make a successful contribution rather than undercut you. If you don't care, we can add the internal version instead, but you may end up making the adjustments before that happens 😆 Whichever comes first! |
No description provided.