RadoslavGeorgiev / rila-framework

A front-end WordPress framework with a set of many extendable class wrappers, inpired by the MVC ideology
GNU General Public License v2.0
18 stars 3 forks source link

Allow builder render method to receive extra data #25

Open JuhG opened 7 years ago

JuhG commented 7 years ago

This would make it possible to pass some extra arguments to the block:

    {{ blocks.render({ some: 'data' }) }}

This might be a very edge case, but I actually had nested content, where it would have been useful to know some info about the parent.

I believe this shouldn't break any existing functionality.

RadoslavGeorgiev commented 7 years ago

Hi @JuhG,

unlike the other two PRs, thank you for which, somehow this does not feel right.

The first thing that catches my attention is the fact that data is modified in the render method. This goes against the principles of concern separation - you end up manipulating data in the template. Of course, in some cases this could be really necessary, but I think that those situations are very isolated. Is there a reason you couldn't do something like the following example?

$post = rila_post();

foreach( $post->content_blocks as $i => $block ) {
    $block[ 'source_object' ] = $post;
    // or
    $block[ 'index' ] = $i;
}

$view = rila_view( 'temlate-name' );
$view->with( 'post', $post' );
$view->render();

A second reason to add the data before the render method is the fact that builders/blocks can also be looped like this like this:

{% for block in blocks %}
    <div id="block-{{ loop.index }}"></div>
    {{ block }}
{% endfor %}

Therefore, even if data is added within the builder, rather directly to the blocks, this should be done via an additional method. And in the context of an additional method, generally I have the feeling it would be better to separate the data of the block and it's context too. Something like this:

class Builder implements \Iterator, \Countable {
    public function set_context( $context ) {
        $this->init();

        foreach( $this->blocks as $block ) {
            $block->set_context( $context );
        }

        return $this;
    }
}

Even better, we could make the set_context method return a modified clone of the builder, in order to make the data more immutable based on the situation.

The only thing I am not sure about is how to handle the context and then passit to the block. Probably we can use the Meta class in order to allow proper mapping and then send it as a context variable to the template of the block.

Can you give an example of what you want to achieve with this context? And how do the ideas above seem to you?

JuhG commented 7 years ago

You are right about the render method not being the right place for it.

Here's the situation that gave me the need for it. There's a flexible field and one of the blocks has another nested flexible field and also a radio to pick a color. The color pick determines the look of the whole outer block, so all the nested blocks should know this and I couldn't find an easy way to achieve it without too many foreach loops. The correct place should be the model, namely the block definition class.

    // Block/Content.php

    public function render($data = [])
    {
        $data['nested']->set_context([
            'color' => 'red',
        ]);

        return rila_view('block.content', $data);
    }

I like the idea of the set_context method, it does what I tried to do, but in a more flexible way. You can override it inside the block classes if you want to avoid it for some blocks.