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

Allow builder render method to receive extra data #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JuhG
Copy link
Contributor

@JuhG JuhG commented Sep 15, 2017

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
Copy link
Owner

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
Copy link
Contributor Author

JuhG commented Sep 16, 2017

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.

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

Successfully merging this pull request may close these issues.

2 participants