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

Make it possible to add extra form_row container attributes #6547

Conversation

nieuwenhuisen
Copy link
Contributor

@nieuwenhuisen nieuwenhuisen commented Oct 28, 2020

Make it possible to add extra form_row container attributes

Implement the Symfony row_attr for form_row containter attributes.
Useful if you need to add extra classes or data attributes on the container. For example:

$builder->add('period', DateRangeType::class, [
    ...
    'field_options_start' => [
        ...
        'row_attr' => [
            'class' => 'col-md-6'
        ],
    ],
    'field_options_end' => [
        ...
        'row_attr' => [
            'class' => 'col-md-6'
        ],
    ]
]);

Output:

<div class="form-group col-md-6" id="sonata-ba-field-container-s5f996c9e7f416_feature_type_14_feature_126_period_start">

</div>

I am targeting this branch, because it's useful from 3.x.

Changelog

### Added
- Added `row_attr` to the form_row container.

@franmomu
Copy link
Member

This is a nice addition, but what about integrate Symfony row_attr?

@nieuwenhuisen nieuwenhuisen force-pushed the feature-form-field-wrapper-attributes branch 4 times, most recently from 205f406 to f0691b5 Compare November 25, 2020 08:37
@franmomu
Copy link
Member

I think we can achieve the same adapting (adding the id) row_attr and there is no need for an extra option.

@nieuwenhuisen nieuwenhuisen force-pushed the feature-form-field-wrapper-attributes branch 2 times, most recently from bbbeb65 to 481ee6a Compare November 25, 2020 12:38
@nieuwenhuisen
Copy link
Contributor Author

This is a nice addition, but what about integrate Symfony row_attr?

Nice, I have implemented the row_attr

@franmomu
Copy link
Member

franmomu commented Nov 26, 2020

Nice!

EDIT: Forget about the thing of tests that do not pass and the submit call, I've created #6627 to fix it.

There is just one thing I fail to understand, tests pass, but if you run just tests/Form/AdminLayoutTestCase.php isolated, Sonata\AdminBundle\Tests\Form\AdminLayoutTestCase::testRowWithErrors fails.

public function testRowWithErrors(): void
{
$form = $this->factory->createNamed('name', TextType::class);
$form->addError(new FormError('[trans]Error 1[/trans]'));
$form->addError(new FormError('[trans]Error 2[/trans]'));
$view = $form->createView();
$html = $this->renderRow($view);
$this->assertMatchesXpath($html, '/div[@class="form-group has-error"][@id="sonata-ba-field-container-name"]');
}

The solution I think is to submit the form because the valid value depends on it. So can you please add $form->submit([])? Something like:

public function testRowWithErrors(): void
{
    $form = $this->factory->createNamed('name', TextType::class);
    $form->addError(new FormError('[trans]Error 1[/trans]'));
    $form->addError(new FormError('[trans]Error 2[/trans]'));
    $form->submit([]);
    $view = $form->createView();
    $html = $this->renderRow($view);

    $this->assertMatchesXpath($html, '/div[@class="form-group has-error"][@id="sonata-ba-field-container-name"]');
}

And also can you please add a test for this? I tried (feel free to used it or make another one):

public function testRowAttr(): void
{
    $form = $this->factory->createNamed('name', TextType::class, '', [
        'row_attr' => [
            'class' => 'foo',
            'data-value' => 'bar',
        ]
    ]);
    $view = $form->createView();
    $html = $this->renderRow($view);

    $this->assertMatchesXpath($html, '//div[@class="foo form-group"][@data-value="bar"][@id="sonata-ba-field-container-name"]');
}

@VincentLanglet
Copy link
Member

#6627 is merged, please rebase

@@ -357,7 +357,7 @@ file that was distributed with this source code.

{% block form_row %}
{% set show_label = show_label ?? true %}
<div class="form-group{% if errors|length > 0 %} has-error{% endif %}" id="sonata-ba-field-container-{{ id }}">
<div{% with {attr: row_attr|merge({id: 'sonata-ba-field-container-'~id, class: (row_attr.class|default('') ~ ' form-group' ~ ((not compound or force_error|default(false)) and not valid ? ' has-error'))|trim})} %}{{ block('attributes') }}{% endwith %}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is hard to read. Please wrap some parts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split this line in some variables

@nieuwenhuisen nieuwenhuisen force-pushed the feature-form-field-wrapper-attributes branch 2 times, most recently from e148e41 to 8d484bd Compare November 26, 2020 12:38
@nieuwenhuisen
Copy link
Contributor Author

Nice!

EDIT: Forget about the thing of tests that do not pass and the submit call, I've created #6627 to fix it.

There is just one thing I fail to understand, tests pass, but if you run just tests/Form/AdminLayoutTestCase.php isolated, Sonata\AdminBundle\Tests\Form\AdminLayoutTestCase::testRowWithErrors fails.

public function testRowWithErrors(): void
{
$form = $this->factory->createNamed('name', TextType::class);
$form->addError(new FormError('[trans]Error 1[/trans]'));
$form->addError(new FormError('[trans]Error 2[/trans]'));
$view = $form->createView();
$html = $this->renderRow($view);
$this->assertMatchesXpath($html, '/div[@class="form-group has-error"][@id="sonata-ba-field-container-name"]');
}

The solution I think is to submit the form because the valid value depends on it. So can you please add $form->submit([])? Something like:

public function testRowWithErrors(): void
{
    $form = $this->factory->createNamed('name', TextType::class);
    $form->addError(new FormError('[trans]Error 1[/trans]'));
    $form->addError(new FormError('[trans]Error 2[/trans]'));
    $form->submit([]);
    $view = $form->createView();
    $html = $this->renderRow($view);

    $this->assertMatchesXpath($html, '/div[@class="form-group has-error"][@id="sonata-ba-field-container-name"]');
}

And also can you please add a test for this? I tried (feel free to used it or make another one):

public function testRowAttr(): void
{
    $form = $this->factory->createNamed('name', TextType::class, '', [
        'row_attr' => [
            'class' => 'foo',
            'data-value' => 'bar',
        ]
    ]);
    $view = $form->createView();
    $html = $this->renderRow($view);

    $this->assertMatchesXpath($html, '//div[@class="foo form-group"][@data-value="bar"][@id="sonata-ba-field-container-name"]');
}

Nice test, I have used it

@nieuwenhuisen nieuwenhuisen force-pushed the feature-form-field-wrapper-attributes branch 2 times, most recently from 36c5412 to be1b0f2 Compare November 26, 2020 12:45
@nieuwenhuisen nieuwenhuisen requested a review from core23 November 26, 2020 12:51
@VincentLanglet VincentLanglet requested a review from a team November 26, 2020 13:26
@nieuwenhuisen nieuwenhuisen force-pushed the feature-form-field-wrapper-attributes branch from be1b0f2 to 72296c7 Compare November 26, 2020 13:59
@VincentLanglet VincentLanglet requested a review from a team November 26, 2020 18:27
@phansys phansys merged commit 88d488f into sonata-project:3.x Nov 27, 2020
@phansys
Copy link
Member

phansys commented Nov 27, 2020

Thank you @nieuwenhuisen!

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

Successfully merging this pull request may close these issues.

5 participants