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.

Sign up for GitHub

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

Jump to bottom

[Autocomplete] required choice types end up not being required. #402

Open
bendavies opened this issue Jul 26, 2022 · 8 comments · May be fixed by #422 or #2282
Open

[Autocomplete] required choice types end up not being required. #402

bendavies opened this issue Jul 26, 2022 · 8 comments · May be fixed by #422 or #2282
Labels
Bug Bug Fix

Comments

@bendavies
Copy link
Contributor

bendavies commented Jul 26, 2022 •
edited
Loading

Hi there,

There seems to be an issue with how tom-select is configured, for required, non-multiple choice types.

Given a form type configuration like

$builder->add('isAttending', ChoiceType::class, [
    'choices' => [
        'Yes' => 'yes',
        'No' => 'no',
        'Maybe' => 'maybe',
    ],
]);

You will be left with a select box like:
image

All well and good, an option is mandatory.

However, convert to autocomplete and you're left with:
image

see the issue? It's clearable because the clear plugin is added, so you can be left with no value.

image

this shouldn't be possible, as it isn't with the simple select box above.

We can try fix fix:

$builder->add('isAttending', ChoiceType::class, [
    'choices' => [
        'Yes' => 'yes',
        'No' => 'no',
        'Maybe' => 'maybe',
    ],
    'autocomplete' => true,
    'required' => true,
]);

this has no effect for 2 reasons:

  1. the symfony/ux-autocomplete controller still adds the clear plugin (while the easyadmin tom-select controller takes requried into considertion, from which symfony/ux-autocomplete was inspired by)
  2. the easy admin version wouldn't work either, because symfony removes the required attribute for select boxes when it's impossible not to submit a value.

Number 2 can be "fixed"/overridden like this:

$builder->add('isAttending', ChoiceType::class, [
    'choices' => [
        'Yes' => 'yes',
        'No' => 'no',
        'Maybe' => 'maybe',
    ],
    'attr' => [
        'required' => true,
    ],
    'autocomplete' => true,
]);

but that would mean we still have to fix the stimulus controller here to not add the clear button if there is a required attribute.

my "fix" above (adding 'attr' => ['required' => true]) is probably too onerous to put on the end user. Maybe the autocomplete form type should do this automatically?

Thanks
@bendavies bendavies changed the title [Autocomplete] Problem with required choice types [Autocomplete] required choice types end up not being required. Jul 26, 2022
@mario-fehr
Copy link

Duplicate of #379 and fixed with #401

@bendavies
Copy link
Contributor Author

bendavies commented Jul 27, 2022 •
edited
Loading

I do not agree that those issues are the same as this one - those are about completely overriding the default plugins setup by symfony/ux-autocomplete.

this issue: I think symfony/ux-autocomplete should be more intelligent/correct out of the box with reguards to required select elements.

@maartendekeizer
Copy link
Contributor

@bendavies Agree, not the same but related.
I think best way is to only add the clear button when the select is not required. Another option is that we never set plugins automaticly but I think that gives bad DX

@weaverryan
Copy link
Member

Hi there!

Really excellent issue and explanation of the problems Ben - thanks for that. As Ben mentioned, the required "form view variable" is not always printed out as a required attribute on the select element, which is why the current solution of looking for that attribute in JavaScript doesn't work. Could we, instead, read the required variable ($view->vars['required']) in the form type extension -

ux/src/Autocomplete/src/Form/AutocompleteChoiceTypeExtension.php

Line 42 in 8e84d49

public function finishView(FormView $view, FormInterface $form, array $options)
- and then use it to pass in a new "value" to the Stimulus controller?

@bendavies
Copy link
Contributor Author

yep, that looks doable. we just need to pass a new required attr which will make it into the template

@weaverryan weaverryan added the Bug Bug Fix label Aug 10, 2022
@bendavies bendavies linked a pull request Aug 11, 2022 that will close this issue
Open
@LucileDT
Copy link

@bendavies do you have any news? 👀

@bendavies
Copy link
Contributor Author

forgot about this. i'll take a look again

@Qronicle
Copy link

Qronicle commented Oct 18, 2024 •
edited
Loading

This is still an issue as far as I can tell. How do we proceed to get the PR merged?

Although thinking about this some more, I feel like the "clearability" of a select is related more to whether there is a placeholder or not. (At least that's how I have always implemented my own js enhanced select elements.)
You can for example have a required select with an empty/placeholder option that forces the user to manually select something. In this case it would still make sense to show the clear button to revert back to the initial, unset state.

I would have to dive into the code to see how the placeholder is deduced, although I wouldn't be opposed to the solution proposed in the PR, as it would still be a major improvement over what there is now.

Edit: I made an attempt at a pull request that implements the placeholder check

@Qronicle Qronicle linked a pull request Oct 18, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Bug Fix
Projects
None yet
Milestone
No milestone
6 participants
@weaverryan @bendavies @maartendekeizer @Qronicle @mario-fehr @LucileDT