feat(select): Adding support for disabled and selected options - front-1590

[planctus]

PR description

Please drop a few lines about the PR: what it does, how to test it, etc.

QA Checklist

In order to ensure a safe and quick review, please check that your PR follow those guidelines:

• I have put the vanilla component as devDependencies
• I have put the specs package as devDependencies
• I have added the components directly used in the twig file (with include or embed) as dependencies
• My component is listed in @ecl-twig/ec-components's dependencies
• My variables naming follow the guidelines (snake case for twig)
• I have provided tests
• I have provided documentation (for the "notes" tab)
• If my local yarn.lock contains changes, I have committed it
• I have given my PR the proper label (pr: review needed to indicate that I'm done and now waiting for a review ,pr: wip to indicate that I'm actively working on it ...)

[kalinchernev]

Having a property extra_attributes inside each option is not what I had in mind with adding a way to spread over attributes from the option. The API is not good in this way IMO. The data should be in the same shape as now and possibly have new attributes like selected in addition. This is how any system would expect the data structure.

One thing coming tom mind is having a destructuring or spreading, but twig does not support these types of syntax. Found this but not a full solution we'll need to specify attributes again and not just pass them along. merge is also not ideal maybe.

I checked how Drupal select template is doing it and it seems to me we won't have the way to make the solution as flexible as I imagined. We'll have to use the selected specifically.

{% spaceless %}
  <select{{ attributes }}>
    {% for option in options %}
      {% if option.type == 'optgroup' %}
        <optgroup label="{{ option.label }}">
          {% for sub_option in option.options %}
            <option value="{{ sub_option.value }}"{{ sub_option.selected ? ' selected="selected"' }}>{{ sub_option.label }}</option>
          {% endfor %}
        </optgroup>
      {% elseif option.type == 'option' %}
        <option value="{{ option.value }}"{{ option.selected ? ' selected="selected"' }}>{{ option.label }}</option>
      {% endif %}
    {% endfor %}
  </select>
{% endspaceless %}

Let's use this approach is my suggestion

[planctus]

This was the initial way i started, the ticket was indeed about the defaultValue, let's say, but options support many more attributes and it would be inconvenient to list them all one by one, the scope was a bit enlarged, then.
This way for instance you would not be able to pass a disabled option, plus you don't have any way to pass all the other allowed attributes for the option element, global attributes and event attributes.
https://www.w3schools.com/tags/tag_option.asp

It seems fine to me to define the required properties independently (label and value) and put all the rest in an array the same way we do for the root element of each component.
This keeps the current structure, just adding an additional key.

A way in between could be to define selected and disabled as properties but still providing an extra_attributes key where to put additional ones or if we consider the "general" attributes out of scope (why?) at least we should also consider the disabled attribute.

What would you prefer, @kalinchernev?

[kalinchernev]

How about trying to read the properties with the attribute function theoretically supported by twing?
I.e. getting data passed to the option without the extra_attributes and setting them via an iterator?

A drawback coming to mind related to this approach is potentially setting malicious code in the markup without knowing.

[planctus]

yeah but the problem is also that you cannot iterate on the object properties in twig unless it's a /Traversable one, anyway, i think here we need to "fix" something, let's stick to the problem, it's enough to fix that.
For this is enough to include "disabled" and "selected", i believe, we do it "statically" as we already do it for the select itself, easy.
I will change the code this way, but still we can wait for this to be merged, if in ECL we are going to change anything in the specs file to "demo" the default value and the disabled option i will adapt this pr accordingly.

[kalinchernev]

Maybe we can support both isDisabled and disabled. I'd suggest the second if we have to pick one.

[kalinchernev]

Just finished up the review on the ECL side. Let's have disabled instead of isDisabled.
It's straight-forward to accommodate on both sides and the adaptation here won't be necessary.