yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Making forms markup more customizable #14937

Closed brussens closed 6 years ago

brussens commented 6 years ago

What steps will reproduce the problem?

The problem with using css styles other than Bootstrap 3.x.x, for example: applying a has-error css class to input element, instead of the container for input element.

What is the expected result?

What do you get instead?

We get as a result:

<div class="form-group has-error">
    <input name="text" type="text" class="form-control">
</div>

instead of this:

<div class="form-group">
    <input name="text" type="text" class="form-control has-error">
</div>

As a result, it is not possible to configure this behavior without completely rewriting some of the core js scripts and widgets.

Additional info

Q A
Yii version 2.0.12
PHP version 7.1.8
Operating system Win 10 x64
brussens commented 6 years ago

At the moment, not very handsome, but, the solution to the problem.

class ActiveForm
{
    const VALIDATING_CSS_SELECTOR_INPUT = 'input';
    const VALIDATING_CSS_SELECTOR_CONTAINER = 'container';

    public $validatingCssSelector;

    protected function getClientOptions()
    {
        $options = [
           ...
            'validatingCssSelector' => $this->validatingCssSelector,
           ...
        ];
        return array_diff_assoc($options, [
            ...
            'validatingCssSelector' => self::VALIDATING_CSS_SELECTOR_CONTAINER,
            ...
        ]);
    }
}

yii.activeForm.js

...
var defaults = {
    ...
    validatingCssSelector: 'container',
    ...
};
...
var methods = {
    ...
    resetForm: function() {
        setTimeout(function () {
                $.each(data.attributes, function () {
                    ...
                    var validatingSelector = undefined;
                    switch(data.settings.validatingCssSelector) {
                        case 'container':
                            validatingSelector = $container;
                            break;
                        case 'input':
                            validatingSelector = $(this.input);
                            break;
                        default:
                            validatingSelector = $container;
                            break;
                    }
                    validatingSelector.removeClass(
                        data.settings.validatingCssClass + ' ' +
                        data.settings.errorCssClass + ' ' +
                        data.settings.successCssClass
                    );
                    $container.find(this.error).html('');
                });
                $form.find(data.settings.errorSummary).hide().find('ul').html('');
            }, 1);
    },
    ...
};
...
var updateInput = function ($form, attribute, messages) {
...
    var validatingSelector = undefined;
    switch(data.settings.validatingCssSelector) {
        case 'container':
            validatingSelector = $container;
            break;
        case 'input':
            validatingSelector = $input;
            break;
        default:
            validatingSelector = $container;
            break;
    }
    if (hasError) {
    ...
        validatingSelector.removeClass(data.settings.validatingCssClass + ' ' + data.settings.successCssClass)
            .addClass(data.settings.errorCssClass);
    } else {
        ...
        validatingSelector.removeClass(data.settings.validatingCssClass + ' ' + data.settings.errorCssClass + ' ')
            .addClass(data.settings.successCssClass);
    }
    ...
};
...

Use ActiveForm:

$form = ActiveForm::begin([
    ...
    'validatingCssSelector' => ActiveForm::VALIDATING_CSS_SELECTOR_INPUT,
    ...
]);
samdark commented 6 years ago

Since the problem pops up again, I think we need to discuss more fundamental solution to custom rendering of forms:

  1. Markup rendering logic should be separated from the form building process.
  2. It should be settable both globally for all forms or for concrete form you're rendering in a view.
brussens commented 6 years ago

I also noticed that all the same it is necessary to determine, use Jquery or native JavaScript. Now everything is put together. Maybe it's time to move to a native JavaScript?

brussens commented 6 years ago

I propose to extract from the ActiveForm extraneous blocks, like "form-group", etc., to generate clean forms, to allow yourself to wrap yourself in the necessary wrappers.

samdark commented 6 years ago

This issue is not about JavaScript. JQuery stuff will be moved out of the core in 2.1.

samdark commented 6 years ago

About extracting blocks, what would be the syntax of these wrappers?

brussens commented 6 years ago

I'm still experimenting with the implementation of the code. But there are ideas, here is one of them:

class ActiveField extends Component
{
    ...
    public $partsOptions;
    ...
    public function render($content = null)
    {
        if ($content === null) {

            $this->scanWrappers();
            ...
        } elseif (!is_string($content)) {
            ...
        }
        ...
    }

    protected function scanWrappers()
    {
        preg_match_all('/\{\{\%?[a-z0-9]+\%?\}\}/', $this->template, $parts);
        $wrappers = array_count_values($parts[0]);
        foreach(array_keys($wrappers) as $part) {
                $cleanName = str_replace(['{{', '%', '}}'], '', $part);
                $options = $this->partsOptions[$cleanName];
                if(preg_match('/^\{\{\%/', $part)) {
                        $this->parts[$part] = Html::beginTag($options['tag'], $options);
                } elseif(strripos($part, '%}}')) {
                        $this->parts[$part] = Html::endTag($options['tag']);
                } else {
                        $this->parts[$part] = Html::tag($options['tag'], '', $options);
                }
        }
    }
}
$form = ActiveForm::begin([
    'fieldConfig' => [
        'template' => "{{%wrapper}}\n{label}\n{input}\n{hint}\n{error}\n{{wrapper%}}",
        'partsOptions' => [
            'wrapper' => [
                'tag' => 'div',
                'class' => 'form-group'
            ]
        ]
    ]
]);
echo $form->field($model, 'attribute', [
    'template' => "{{%wrapper}}\n{label}\n{{wrapper}}\n{input}\n{{wrapper%}}\n{hint}\n{error}",
    'partsOptions' => [
        'wrapper' => [
            'tag' => 'section',
            'class' => 'form-section'
        ]
    ]
])->textInput();

P.S.: For the code do not scold, wrote on the knee.

brussens commented 6 years ago

@samdark , today, I put together an approximate version with the preservation of BC. It seems like it's convenient and in a single style. HTML tags now do not need to be written in templates. Threw an example in the github gist, that would not clog the discussion. See https://gist.github.com/brussens/c0ee9fae6edc97b11f260a7c404652c2 How do you like this implementation?

samdark commented 6 years ago

How would you customize that to generate bootstrap3 markup, bootstrap4 markup or foundation markup?

brussens commented 6 years ago

@samdark , in the variant I proposed, I saw the opportunity to manipulate the default parts. It turns out, slightly changing the proposed version of me, you can do something like this:

// Bootstrap 3
$form = ActiveForm::begin([
    'fieldConfig' => [
        'template' => "{%form-group}\n{label}\n{input}\n{hint}\n{error}{form-group%}",
        'hasErrorMarkedPart' => 'form-group',
        'hasErrorCssClass' => 'has-error',
        'partsOptions' => [
            'form-group' => [
                'tag' => 'div',
                'options' => [
                    'class' => 'form-group'
                ]
            ]
        ]
    ]
]);
// Bootstrap 4
$form = ActiveForm::begin([
    'fieldConfig' => [
        'template' => "{%form-group}\n{label}\n{input}\n{hint}\n{error}{form-group%}",
        'hasErrorMarkedPart' => 'input',
        'hasErrorCssClass' => 'is-invalid',
        'partsOptions' => [
            'form-group' => [
                'tag' => 'div',
                'options' => [
                    'class' => 'form-group'
                ]
            ]
        ]
    ]
]);

Also, we can add the possibility of using lables as a wrapper:

echo $form->field($model, 'attribute', [
    'template' => "{%form-group}\n{%label}\n{input}\n{label%}\n{hint}\n{error}{form-group%}",
    'partsOptions' => [
        'form-group' => [
            'tag' => 'div',
            'options' => [
                'class' => 'form-group'
            ]
        ]
    ]
]);
brussens commented 6 years ago

@samdark , after a little thought, it turned out to make the form's scripts work as well as write tags directly in the template. Updated the gist https://gist.github.com/brussens/c0ee9fae6edc97b11f260a7c404652c2

brussens commented 6 years ago

All the work of scripts is done using the data-attributes, so the client side doesn't know much about server side, just as the server side doesn't know anything about the client side.

samdark commented 6 years ago

Yes, using data-attributes is a good idea.

brussens commented 6 years ago

What do you think about the template syntax that I proposed?

samdark commented 6 years ago

Well, it's debatable. I don't think I like such solution much but it's OK.

brussens commented 6 years ago

@samdark , here came the idea of how to simplify the code, the following result: ActiveField changes

ActiveField extends Component
{
    public $wrappers = [];

    /**
     * @var string|array
     */
    public $validationStatusElements = 'input';

    public function render($content = null)
    {
        if ($content === null) {
            $this->renderWrappers();
            if (!isset($this->parts['{input}'])) {
                $this->textInput();
            }
            if (!isset($this->parts['{label}'])) {
                $this->label();
            }
            if (!isset($this->parts['{error}'])) {
                $this->error();
            }
            if (!isset($this->parts['{hint}'])) {
                $this->hint(null);
            }
            $content = strtr($this->template, $this->parts);
        } elseif (!is_string($content)) {
            $content = call_user_func($content, $this);
        }

        return $this->begin() . "\n" . $content . "\n" . $this->end();
    }

    private function renderWrappers()
    {
        if(!empty($this->wrappers)) {
            foreach ($this->wrappers as $name => $config) {
                $this->renderWrapper($name, $config);
            }
        }
    }

    private function renderWrapper($name, $config)
    {
        $begin = '{%' . $name . '}';
        $end = '{' . $name . '%}';
        if((strpos($this->template, $begin) !== false) && (strpos($this->template, $end) !== false)) {
            if($this->isValidationStatusElement($name)) {
                $options['data-validation-selector'] = Html::getInputId($this->model, $this->attribute);
            }
            $this->parts[$begin] = Html::beginTag($config['tag'], $config['options']);
            $this->parts[$end] = Html::endTag($config['tag']);
        }
    }

    private function isValidationStatusElement($name)
    {
        if(is_array($this->validationStatusElements)) {
            return in_array($name, $this->validationStatusElements);
        } else {
            return $name === $this->validationStatusElements;
        }
    }
}

Use in view

$form = ActiveForm::begin([
    'fieldConfig' => [
        'template' => "{%test-field}\n{label}\n{input}\n{hint}\n{error}\n{test-field%}",
        'validationStatusElements' => [
            'test-field',
            'input'
        ],
        // Or for one element in a row
        // 'validationStatusElements' => 'test-field'
        'wrappers' => [
            'test-field' => [
                'tag' => 'section',
                'options' => [
                    'class' => 'testing-field-class'
                ]
            ]
        ]
    ]
]);
samdark commented 6 years ago

Closed recently in https://github.com/yiisoft/yii2/commit/b3130be7bab8a835f226e9a03fc265dab616b828