Add support for optgroup-tags for select-fields #1778
Conversation
In more complex selects we may want to group options by some criteria. HTML has the <optgroup>-tag for that purpose. However in Order to not duplicate code, I've to move the handling of options to the OptGroup-Class completely. Known Issues: ------------- * We may have more than one option with the same value and therefore more than one option may be marked as selected.
|
@micgro42, thanks for your PR! By analyzing the history of the files in this pull request, we identified @splitbrain to be a potential reviewer. |
|
haven't looked at the code, yet. but from your description I have a question. what happens if two options are selected at the same time? I guess the browser will only select one. but is it even allowed? can we prevent it? |
|
Multiple selected option would not be valid HTML.
| /** | ||
| * @param string $name The name of this form element | ||
| * @param string $options The available options | ||
| * @param string $label The label text for this element (will be autoescaped) |
| $attrs = ''; | ||
| if (is_array($val['attrs'])) { | ||
| array_walk($val['attrs'],function (&$aval, $akey){$aval = hsc($akey).'="'.hsc($aval).'"';}); | ||
| $attrs = join(' ', $val['attrs']); |
There was a problem hiding this comment.
isn't this basically doing what buildAttributes() does? Only much harder to read?
| public function setValue($value) { | ||
| $this->value = $value; | ||
| return isset($this->options[$value]); | ||
| } |
There was a problem hiding this comment.
It's a bit unclear what this function does. It seems to store the currently selected value? What if this optgroup is part of a multiselect? What exactly is the return value?
| $this->optGroups = array(); | ||
| foreach ($optGroups as $label => $options) { | ||
| $this->addOptGroup($label, $options); | ||
| } |
There was a problem hiding this comment.
should it return $this here?
| $keys = array_keys($this->options); | ||
| $this->value = (string) array_shift($keys); | ||
| $this->value = $this->getFirstOption(); | ||
| $this->setValueInOptGroups($this->value); |
There was a problem hiding this comment.
how are we handling multi selects? it seems this only works with a single value?
There was a problem hiding this comment.
We are not, we are throwing an exception when setting the multiple-attribute. I removed my erroneous handling of multi selects in 45082b9.
We should consider a new PR for adding multi-select-functionality.
| $html .= '<option' . $selected . ' value="' . hsc($key) . '" '.$attrs.'>' . hsc($val['label']) . '</option>'; | ||
| } | ||
| $html .= $this->options->toHTML(); | ||
| $html = array_reduce($this->optGroups, function($html, OptGroup $optGroup) {return $html . $optGroup->toHTML();}, $html); |
There was a problem hiding this comment.
I think it's a bit weird that not all options are handled through the optgroups in $this->optGroups but that there are two places. I would expect the "unlabeled" optgroup to be in that array as well.
We currently throw an exception with setting the multiple-attribute is attempted. If we want to implement this functionality, it might be more sensible to make an extra commit for that.
PHP 5.6 and below throw a strict standards warning at the changed lines. An intermediate variable is introduced to avoid this warning. PHP 7+ changes the severity of this warning to E_NOTICE which is suppressed by DokuWiki. This error was introduced in #1778
In more complex selects we may want to group options by some criteria. HTML has the
<optgroup>-tag for that purpose.However in order to not duplicate code, I have chosen to move the handling of options to the
OptGroup-class.ToDo
Known Issues:
selected-attribute.This also fixes #1776