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

[LiveComponent] Checkbox check/uncheck doesn't trigger component update #906

Closed
welcoMattic opened this issue May 27, 2023 · 3 comments · Fixed by #910
Closed

[LiveComponent] Checkbox check/uncheck doesn't trigger component update #906

welcoMattic opened this issue May 27, 2023 · 3 comments · Fixed by #910
Labels
Bug Bug Fix

Comments

@welcoMattic
Copy link
Member

Based on the following documentation I tried to build a simple list filter based on checkboxes:

Code

FilterMachines.php

<?php

namespace App\Twig\Components;

use Symfony\UX\LiveComponent\Attribute\AsLiveComponent;
use Symfony\UX\LiveComponent\Attribute\LiveProp;
use Symfony\UX\LiveComponent\DefaultActionTrait;

#[AsLiveComponent('filter_machines')]
class FilterMachines
{
    use DefaultActionTrait;

    #[LiveProp(writable: true)]
    public array $types = ['HYDRAULIC', 'AUTOMATIC', 'REPRISE'];

    public function __construct(
    ) {
    }

    public function getMachines(): array
    {
        return array_filter([
            ['name' => 'Machine 1', 'type' => 'HYDRAULIC'],
            ['name' => 'Machine 2', 'type' => 'AUTOMATIC'],
            ['name' => 'Machine 3', 'type' => 'REPRISE'],
        ], fn($machine) => in_array($machine['type'], $this->types));
    }
}

filter_machines.html.twig

<div {{ attributes }}>
    <label for="type-hydraulic">hydraulic</label>
    <input type="checkbox" data-model="types" id="type-hydraulic" value="HYDRAULIC">
    <label for="type-automatic">automatic</label>
    <input type="checkbox" data-model="types" id="type-automatic" value="AUTOMATIC">
    <label for="type-reprise">reprise</label>
    <input type="checkbox" data-model="types" id="type-reprise" value="REPRISE">
    {% if computed.machines|length > 0 %}
        <ul>
            {% for machine in computed.machines %}
                <li>
                    <strong>{{ machine.name }}</strong>
                    <p>{{ machine.type }}</p>
                </li>
            {% endfor %}
        </ul>
    {% else %}
        <div>Ah! No machines found matching "{{ types }}"</div>
    {% endif %}
</div>

index.html.twig (rendered by a dead simple FooController)

{% extends 'base.html.twig' %}

{% block title %}Hello HomeController!{% endblock %}

{% block body %}
    {{ component('filter_machines') }}
{% endblock %}

To reproduce:

  • Run the previous code
  • Uncheck any checkbox
  • See that nothing happens (no AJAX request, no component render)

Versions:

Symfony 6.2
Webpack Encore Bundle 1.17
UX LiveComponent 2.9

@welcoMattic welcoMattic added the Bug Bug Fix label May 27, 2023
@welcoMattic
Copy link
Member Author

This test fails


it('sends correct data for array valued checkbox fields without form with initial data', async () => {
        const test = await createTest({ check: ['foo'] }, (data: any) => `
            <div ${initComponent(data)}>
                <label>
                    Checkbox 1: <input type="checkbox" data-model="check[]" value="foo" ${data.check.indexOf('foo') > -1 ? 'checked' : ''} />
                </label>

                <label>
                    Checkbox 2: <input type="checkbox" data-model="check[]" value="bar" ${data.check.indexOf('bar') > -1 ? 'checked' : ''} />
                </label>
                
                Checkbox 1 is ${data.check.indexOf('foo') > -1 ? 'checked' : 'unchecked' }
            </div>
        `);

        const check1Element = getByLabelText(test.element, 'Checkbox 1:');
        const check2Element = getByLabelText(test.element, 'Checkbox 2:');

        // only 1 Ajax call will be made thanks to debouncing
        test.expectsAjaxCall()
            .expectUpdatedData({ 'check': ['bar'] });

        await userEvent.click(check2Element);
        await userEvent.click(check1Element);

        await waitFor(() => expect(test.element).toHaveTextContent('Checkbox 1 is unchecked'));

        expect(test.component.valueStore.getOriginalProps()).toEqual({check: ['bar']});
    });

But this one pass


it('sends correct data for array valued checkbox fields without form with initial data', async () => {
        const test = await createTest({ form: { check: ['foo']} }, (data: any) => `
            <div ${initComponent(data)}>
                <label>
                    Checkbox 1: <input type="checkbox" data-model="form[check][]" value="foo" ${data.form.check.indexOf('foo') > -1 ? 'checked' : ''} />
                </label>

                <label>
                    Checkbox 2: <input type="checkbox" data-model="form[check][]" value="bar" ${data.form.check.indexOf('bar') > -1 ? 'checked' : ''} />
                </label>
                
                Checkbox 1 is ${data.form.check.indexOf('foo') > -1 ? 'checked' : 'unchecked' }
            </div>
        `);

        const check1Element = getByLabelText(test.element, 'Checkbox 1:');
        const check2Element = getByLabelText(test.element, 'Checkbox 2:');

        // only 1 Ajax call will be made thanks to debouncing
        test.expectsAjaxCall()
            .expectUpdatedData({ 'form.check': ['bar'] });

        await userEvent.click(check2Element);
        await userEvent.click(check1Element);

        await waitFor(() => expect(test.element).toHaveTextContent('Checkbox 1 is unchecked'));

        expect(test.component.valueStore.getOriginalProps()).toEqual({form: {check: ['bar']}});
    });

So, it looks like the model name matter, but it shouldn't.

@Jim56450
Copy link

Hi welcoMatic !

I think the trouble comes from ValueStore.get(name) : it doesn't return a copy of the array which is then modified.
It's called via function getValueFromElement(element, valueStore) and the array is updated (or not) via return getMultipleCheckboxValue(element, modelValue);

Changing ValueStore.get from

        if (this.props[normalizedName] !== undefined) {
            return this.props[normalizedName];
        }

to

        if (this.props[normalizedName] !== undefined) {
            return [...this.props[normalizedName]];
        }

will make it work.


The change could otherwise be done in getMultipleCheckboxValue function instead of in ValueStore.get function.

@Jim56450
Copy link

Answering myself, the proper way for ValueStore.get modification would be :

        if (this.props[normalizedName] !== undefined) {
            if (Array.isArray(this.props[normalizedName])) {
                return [...this.props[normalizedName]];
            }
            return this.props[normalizedName];
        }

symfony-splitter pushed a commit to symfony/ux-live-component that referenced this issue May 30, 2023
…dling (welcoMattic)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[LiveComponent] Fix array valued checkboxes change event handling

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Tickets       | Fix symfony/ux#906
| License       | MIT

As described in the linked issue, the event `change` of checkboxes weren't fully triggered (it didn't trigger AJAX request). It was because of the always-equals values of the pre-checked value attribute of the element and the after-change-event value of the element.

Thanks `@Jim56450` for the help!

Commits
-------

0e94fec4 [LiveComponent] Fix array valued checkboxes change event handling
web9app6 added a commit to web9app6/ux that referenced this issue Aug 1, 2023
…dling (welcoMattic)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[LiveComponent] Fix array valued checkboxes change event handling

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Tickets       | Fix symfony/ux#906
| License       | MIT

As described in the linked issue, the event `change` of checkboxes weren't fully triggered (it didn't trigger AJAX request). It was because of the always-equals values of the pre-checked value attribute of the element and the after-change-event value of the element.

Thanks `@Jim56450` for the help!

Commits
-------

0e94fec4 [LiveComponent] Fix array valued checkboxes change event handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants