PrestaShop / prestafony-project

Some resources to help you migrate PrestaShop to Symfony 3
https://github.com/PrestaShop/prestafony-project/projects/1
11 stars 8 forks source link

Grid columns templating #40

Closed sarjon closed 4 years ago

sarjon commented 6 years ago

Column content templates as blocks

At the moment, column content is rendered by locating correct template by using column's type like this:

{{ include('@PrestaShop/Admin/Common/Grid/Columns/'~column.type~'.html.twig', {'grid': grid, 'column': column, 'row': row}) }}

What if we allow defining twig block for columns? Here's an example:

{# columns_layout.html.twig #}

{# Default column template #}
{% block column_default_content %}
    {{ row[column.id] }}
{% endblock %}

{# Template for column with type of "employee_name_with_avatar" #}
{% block column_employee_name_with_avatar_content %}
    {% set employeeName, employeeImage = row.employee, 'http://profile.prestashop.com/'~row.email|url_encode~'.jpg'%}
    <span class="employee_avatar_small">
        <img class="img rounded-circle" alt="{{ employeeName }}" src="{{ employeeImage }}" height="32" width="32" />
    </span>
    {{ employeeName }}
{% endblock %}

So inside template we can render column content like so:

{{ column_content(column, grid) }}

Page specific column customization

Lets say our column has type employee_name_with_avatar, so column_content() would try to figure out template for it, first by checking if column_employee_name_with_avatar_content block exists and if so, it renders it. Otherwise it would fall back to default template or throw an error.

What if column_content() could also check for page specific column blocks. Lets say we want to customize only employee_name_with_avatar column content for Logs page? So column_content() could try to figure out correct block by:

  1. Checking if page specific block exists, example: {% block column_logs_employee_name_with_avatar_content %}. Note: we are using grid specific id in column block name
  2. Checking if column specific block exists, example: {% block column_employee_name_with_avatar_content%}`.
  3. Render using default block or throw error, because block is not found.

Headers and filters column customization using blocks

We could use similar approach too:

{{ column_header(column, grid) }}
{{ column_filter(column, grid) }}

So block for both header and filters could be defined, example:

{# default template for column header #}
{% block column_default_header %}
   {{ column.name }}
{% endblock %}

{# template for column with type "employee_name_with_avatar" #}
{% block column_employee_name_with_avatar_header %}
    <span data-some-attribute="some value" class="custom-class">
        {{ column.name }}
    </span>
{% endblock %}

{# Logs page specific template for column with type "employee_name_with_avatar" #}
{% block column_logs_employee_name_with_avatar_header %}
     {{ column.name }} (Logs page)
{% endblock %}

Blocks defined by modules

Every module should be able to load both their own and/or customized column blocks. It could be done using hooks or some other implementation.

sarjon commented 6 years ago

ping @mickaelandrieu

mickaelandrieu commented 6 years ago

Blocks defined by modules

I disagree for hooks. Templating should be done using templating loaders only.

sarjon commented 6 years ago

Templating should be done using templating loaders only.

:+1:

what do you think about this approach to use blocks instead of separate template files?

mickaelandrieu commented 6 years ago

I think this make the system more difficult to be learned by developers and I see no extra value compared to the same system using separated files.

Let's imagine the same behavior with colum_* functions, but in the end we include a template: what are the gains, what are the pros compared to use dynamic block names?

sarjon commented 6 years ago

i guess its the same, except you can define many blocks in single file.

in the end, i agree with you, so may close this issue. :)

mickaelandrieu commented 6 years ago

nope, because I agree on the improvements you suggest: do you think it is possible to do it using a template for every column/column header/column filter?

I'm also in favor of column_* functions, this may be something similar to form_* functions so both PrestaShop and Symfony developers will be easy with this way to do (and we hide the complexity a little bit, which is not bad ^^)

sarjon commented 6 years ago

we can do that, here's a PR that could start it: https://github.com/PrestaShop/PrestaShop/pull/9281

however, heres some things that could be discussed:

  1. what do you think if templates would be suffixed with *_content.html.twig? So we maybe have different templates in same directory, example: employee_name_with_avatar_content.html.twig, employee_name_with_avatar_header.html.twig and employee_name_with_avatar_filter.html.twig

  2. what if module could specify column type like modulename:column_type? in this case we could optimize to look for template directly in that module's directory and nowhere else.

  3. how do we know where modules keeps it's templates? it can be modules/modulename/templates/my_column_template.html.twig or modules/modulename/views/admin/columns/my_column_template.html.twig or anything else. how do we make it configurable?

  4. do you think there should be any default template if template by column type cannot be found?

mickaelandrieu commented 6 years ago
  1. Ok, make sense :+1:

  2. this optimizes nothing as Twig is looking for templates in modules first and will loop on every module... what is the cost (ie performance graph with blackfire that justify such an optimization here?) This also means an another module can't override a Grid column template... not sure it's a good idea.

  3. In fact, we know: it's documented and it's in modulename/views/PrestaShop/ folder or this won't be found. I don't see the gain of making it configurable.

  4. Yes, I want to rely on Simple|DataColumn if nothing is found.

sarjon commented 6 years ago

okay, but regarding 2nd point, 2 or more totally different modules may (and probably will at some point) create columns with same type, in that case, it will render first matching template, isnt it an issue?

mickaelandrieu commented 6 years ago

it will render first matching template, isn't it an issue?

No, as it is the behavior of every override of a template. Everything should behave the same, we must provide a consistent experience.

sarjon commented 6 years ago

@mickaelandrieu after playing with column customizing i noticed a few things:

  1. we can only store column templates in one directory: @PrestaShop/Admin/Common/Grid/Columns/{column_type}.html.twig. Problem is that there will be a lot of templates in single directory and they cannot be grouped. By using twig blocks we can solve it.

  2. By using block we can store column blocks in single file, for example:

    
    {# PrestaShop/Admin/Common/Grid/Columns/AdvancedParameters/Logs/logs.html.twig #}

{% block column_employee_with_avatar_widget %} // render content {% endblock %}

{% block column_employee_with_avatar_filter %} // render column filter {% endblock %}

{% block column_employee_with_avatar_header %} // render column header {% endblock %}


3. We need to be able to customize not only grid specific columns, but also specific column in grid (because we can multiple `DataColumn` but for different fields) for , for example:
```twig
{# first we try to load column specific block for specific grid #}
{% block column_{grid_id}_{column_id}_{column_type}_widget %}{% endblock %}
{# then we try to load grid specific column block #}
{% block column_{grid_id}_{column_type}_widget %}{% endblock %}
{# then we try to load column specific column block #}
{% block column_{column_type}_widget %}{% endblock %}
{#  and last we load default column block if others have failed #}
{% block column_widget %}{% endblock %}
mickaelandrieu commented 6 years ago

Hi @sarjon,

just noticed this last comment from you: interesting thoughts.

Looking at 3) the issue is that every block will be rendered at the same time when we want only one (the more specific if it is defined ) to be rendered.

Looking at 1) it's not really an issue, I'm totally fine with having 30 templates of columns in Admin/Common/Grid/Columns/ if we need to implement 30. Look at Form component, every form type is stored in Type namespace, it's consistent => https://github.com/symfony/form/tree/master/Extension/Core/Type

For 2) I'm not "against" this idea but I want 1 and only 1 method to override a template: this ease the use and learning of PrestaShop and this ease the maintenance, too :)

I have some free time today, I'll play with it

sarjon commented 6 years ago

Looking at 3) the issue is that every block will be rendered at the same time

not really, what i mean is we should have plain old checks, to determine what should be rendered, like:

{% if block("very_specific_block") is defined %}
{# render that block #}
{% elseif block("less_specific_block") is defined %}
{# render block #}
{% endif %}

but i dont know if this can be easily implemented.

Looking at 1) it's not really an issue, I'm totally fine with having 30 templates

yup, thats totally not a problem to have templates for every column type in same directory. But how i see, the problem comes when we start adding specific templates and there can be another 30 or more templates for that in same directory, so it would look like a mess.

Lets say we are building Logs grid and we want to customize Error Code column. So instead of adding new template in Common/Grid/Columns/logs_error_code.html.twig i'd like to put it under Admin/Configure/AdvancedParameters/LogsPage/Grid/Columns/logs_error_code.html.twig. So we are able to store these page/grid specific templates outside Grid, wdyt?

mickaelandrieu commented 6 years ago

i'd like to put it under Admin/Configure/AdvancedParameters/LogsPage/Grid/Columns/logs_error_code.html.twig. So we are able to store these page/grid specific templates outside Grid, wdyt?

Why not. How do you plan to allow that? And how to do it why we need to make all grids works the same way and to migrate them as fast as possible?

sarjon commented 6 years ago

How do you plan to allow that?

thats an issue, i dont have a clear solution for that... :/

i had this idea to configure it using yaml, but i think its too much work to make it work or maybe not even worth implementing it. For example:

# app/config/grid.yml

grid:
    templating:
        column:
            # by default it could use default_content_path and column type to render column
            default_content_path: '@PrestaShop\Admin\Common\Column\Content'
            default_header_path: '@PrestaShop\Admin\Common\Column\Header'
            custom_content_paths:
                # custom column content template for Logs grid Error Code column
                logs_error_code: '@LogsPage/Grid/Columns/logs_error_code.html.twig'

so we could skip resolving correct template as it is always configured.