Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local value support (Non Ajax local choices) #68

Closed
wants to merge 3 commits into from

Conversation

Schyzophrenic
Copy link
Contributor

@Schyzophrenic Schyzophrenic commented Feb 18, 2017

New Feature: Support of non Ajax field type

This PR is linked to the Feature Request #23. I give it a try and it seems to work OK in my current project.
Please note this PR is based on my previous one adding "tag: support for single entity (#67).

Let me know if you need anything else.
I hope you will have time to merge these requests soon as I am getting further away from your base code, and would like to avoid maintaining a separate branch :)

PS: Any comments on the code are welcome

@tetranz
Copy link
Owner

tetranz commented Feb 19, 2017

I tried this but there is a bit of a problem. I haven't put anything in choices so it's null so I guess should work as normal via ajax. My form is not showing the existing values. The elements still work in that I can start typing and it displays the list from ajax but the value is not being displayed when select2 is not "open".

I have a test form like this with two multiples and one single.

screenshot from 2017-02-18 21-57-15

Apply your patch, refresh and ...

screenshot from 2017-02-18 21-57-48

@Schyzophrenic
Copy link
Contributor Author

Schyzophrenic commented Feb 19, 2017

@tetranz Hmm sorry about that...
To be honest I was wondering whether we should have a test part for this bundle with a test page trying the different cases...
Can you try to replace the option value in fields.html.twig?

<option value="{{ id }}" selected="selected">{{ label }}</option>

It might be a bit more complex than this though... Would you mind sharing your test data so I can fix it on my end?

@tetranz
Copy link
Owner

tetranz commented Feb 19, 2017 via email

@Schyzophrenic
Copy link
Contributor Author

@tetranz Let me know if I can help (and how :))

@tetranz
Copy link
Owner

tetranz commented Feb 19, 2017

Unfortunately that didn't help. choices is null in the template. If I get some time I'll try to do some debugging.

@tetranz
Copy link
Owner

tetranz commented Feb 20, 2017

The problem is simply that for remote data, the select needs to iterate over value and for local it needs to iterate over choices. The other difference is that for remote, every option needs selected="selected" because only the existing values are actually rendered whereas for local, I assume only those that are selected should have selected="selected".

I guess we could do {% set choices = value %} above so it gets into the same variable but we still need to deal with the selected difference. Maybe we need a whole different block for the select depending on whether it's doing local or remote data.

@JulienMaille
Copy link

Sorry to interrupt, is the point of this PR to display a list of options before the user start typing in the field?
Is this PR the only way to do that?

@Schyzophrenic
Copy link
Contributor Author

Yes, you're right, we could do with some
proper tests. Unfortunately I don't have much free time right now.

@tetranz , Would that make sense to just add a Controller and some views to test the rendering of the elements (plus potentially some automated tests)?

Let me know if you would like to get this included in the bundle. That might be helpful to validate the PR as well :)

@itaelle
Copy link

itaelle commented Mar 21, 2017

Any update on when would this request be merged?

@tetranz
Copy link
Owner

tetranz commented Mar 22, 2017

I left a comment and then deleted it. I thought it was talking about something that had been merged.

To answer the question, when will it be merged? Unfortunately, this PR doesn't work. It kills the ajax behavior and, as @mitch10593 points out, it removes good working code. Unfortunately I don't have time to fix it. I'm happy to merge a PR to include local value support provided it doesn't break other functionality.

@tetranz
Copy link
Owner

tetranz commented Mar 22, 2017

I'm surprised that local value support is such a sought after feature. Maybe I'm missing something but isn't it just as easy to use a standard choice field and one line of Javascript / jQuery to add the Select2 goodness?

@JulienMaille
Copy link

@tetranz In reaction to your last comment, may I reiterate my question:
is the point of this PR to display a list of options before the user start typing in the field?
Is this PR the only way to do that?

@Schyzophrenic
Copy link
Contributor Author

Schyzophrenic commented Mar 29, 2017 via email

@Azraelir
Copy link

Azraelir commented Sep 8, 2019

any update on this one?

it would be nice to display a list of options before the user start typing in the field.

@DubbleClick
Copy link
Contributor

DubbleClick commented Sep 16, 2019

You can work around this by setting minimum input length to 0. Or add an option 'default_choices' to the AjaxEntityType locally and extend the javascript method as documented. Other than that, you'll have to wait for a merge. Which won't happen until the PR is fixed.

@tetranz
Copy link
Owner

tetranz commented Sep 16, 2019

I'll try to take another look at this this week. Last time I tried it (long time ago now), it broke existing functionality.

I can probably deal with the merge conflict if that's the only problem but I think it's more than that.

I'm happy to include this feature if someone comes up with a PR that works without breaking BC.

@DubbleClick
Copy link
Contributor

Looking at the code, this actually looks like it's meant to support ONLY local values, not in combination with the ajax. What the hell? That's not what the bundle is for. If you want select2 fields, just override Symfonys EntityType widget and pass the 'choices' option in the FormBuilder there.

If anything, I feel like this PR should be changed in the way that it adds the functionality of default choices (before the user started typing) to be passed with a 'default_choices' option or similar.
@tetranz would you feel that's appropriate, or should that case be handled with minimum_input_length => 0 instead?

@DubbleClick
Copy link
Contributor

DubbleClick commented Sep 16, 2019

Okay, so if someone wants select2 functionality for just local choices, simply override the choice_widget in Form/fields.html.twig (corresponds to ChoiceType, which is the parent of EntityType).

{%- block choice_widget_collapsed -%}
    {% set attr = attr|merge({class: (attr.class|default('') ~ ' select2')|trim}) %}
    <select {{ block('widget_attributes') }}{% if multiple %} multiple="multiple"{% endif %} style="width: 100%;">
        {%- if placeholder is not none -%}
            <option value=""{% if required and value is empty %} selected="selected"{% endif %}>{{ placeholder != '' ? placeholder|trans({}, translation_domain) }}</option>
        {%- endif -%}
        {%- set options = choices -%}
        {{- block('choice_widget_options') -}}
    </select>
{%- endblock choice_widget_collapsed -%}

and then call select2 in your javascript:

    // select2
        $('.select2:not(.no-select2)').each(function () {
            $(this).select2({
                'placeholder': function () {
                    $(this).data('placeholder');
                }
            });
        });

and that's it. You don't need this bundle. Edit: you can alternatively override just the EntityType widget, or create your own Form type with EntityType as a parent, for which you then override the widget.

Edit: Coming to think of it, you literally only need the javascript code and add a EntityType field like this:

$builder->add('something', EntityType::class, [
    'class' => SomeThing::class,
    'attr' => ['class' => 'select2']
]);

@Schyzophrenic
Copy link
Contributor Author

This PR is no longer relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants