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

[Events] Sylius template event forms #13798

Merged
merged 12 commits into from
Mar 31, 2022

Conversation

SirDomin
Copy link
Contributor

@SirDomin SirDomin commented Mar 25, 2022

Q A
Branch? 1.10
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

Introduce sylius template events on forms for easy overwriting templates.

@SirDomin SirDomin requested a review from a team as a code owner March 25, 2022 06:54
@probot-autolabeler probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Mar 25, 2022
@SirDomin SirDomin force-pushed the sylius-template-event-forms branch from 47c845d to 0d0cd17 Compare March 25, 2022 07:17
{{ form_errors(form) }}

<div class="ui two column stackable grid">
{{ sylius_template_event('sylius.admin.channel.before_form', _context) }}
Copy link
Member

Choose a reason for hiding this comment

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

Can you check if the "form" variable is present inside of _context? Also, if some additional attribute is required to call it, lets add it explicitly, to be sure people may use it as in normal form template

E.g
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, that could be done like this, but in sylius plus we are using more than just forms, this way we allow user to use any variable in that event. I think its more customize friendly than sending just the form.

Copy link
Member

Choose a reason for hiding this comment

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

But is the form available straight from the template or behind some additional variable? I

Copy link
Contributor

Choose a reason for hiding this comment

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

Directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's some examples of it being used it other places: https://github.com/Sylius/Sylius/search?l=Twig&q=_context

@vvasiloi
Copy link
Contributor

vvasiloi commented Mar 25, 2022

Great initiative! I'm not a fan of before / after hooks, especially in complex templates like forms.
From my experience customizing forms, I rarely wanted to just append / prepend something to a form.
I would rather partition the forms in smaller pieces and display each piece with an event then let the events system do it's job.
Here's a good example of it:

  • sylius.shop.layout.header in @SyliusShop/layout.html.twig
    sylius.shop.layout.header:
        blocks:
            header:
                template: "@SyliusShop/_header.html.twig"
                priority: 20
            after_header_legacy:
                template: "@SyliusUi/Block/_legacySonataEvent.html.twig"
                priority: 15
                context:
                    event: sylius.shop.layout.after_header
            menu:
                template: "@SyliusShop/Layout/Header/_menu.html.twig"
                priority: 10
  • sylius.shop.layout.header.grid in @SyliusShop/_header.html.twig
    sylius.shop.layout.header.grid:
        blocks:
            logo:
                template: "@SyliusShop/Layout/Header/_logo.html.twig"
                priority: 30
            content:
                template: "@SyliusShop/Layout/Header/_content.html.twig"
                priority: 20
            cart:
                template: "@SyliusShop/Layout/Header/_cart.html.twig"
                priority: 10
  • sylius.shop.layout.header.content in @SyliusShop/Layout/Header/_content.html.twig

@lchrusciel
Copy link
Member

Looking for @vvasiloi and @GSadee answers we have a consensus on how to do it:

sylius_ui:
    events:
        sylius.admin.channel.form_content:
            blocks:
                content:
                    template: '@SyliusAdmin/Channel/Form/_content.html.twig' # inside of it just enough of HTML to have columns layout
                    priority: 10
        sylius.admin.channel.form_columns:
            blocks:
                first_column:
                    template: '@SyliusAdmin/Channel/Form/_first_column.html.twig' # inside of it just enough of HTML to single column wrapping + new event
                    priority: 20
                second_column:
                    template: '@SyliusAdmin/Channel/Form/_second_column.html.twig' # inside of it just enough of HTML to single column wrapping + new event
                    priority: 10
        sylius.admin.channel.first_form_column:
            blocks:
                general:
                    template: '@SyliusAdmin/Channel/Form/FirstColumn/_general.html.twig' # content from H4 with sylius.ui.general + block below it
                    priority: 20
                money:
                    template: '@SyliusAdmin/Channel/Form/FirstColumn/_money.html.twig' # content from H4 with sylius.ui.money + block below it
                    priority: 10

Probably could be even more granural, but that would be great improvement already IMHO

@SirDomin SirDomin added the DX Issues and PRs aimed at improving Developer eXperience. label Mar 30, 2022
@SirDomin SirDomin force-pushed the sylius-template-event-forms branch from 4b6acb5 to 4eba41b Compare March 30, 2022 10:02
@SirDomin SirDomin force-pushed the sylius-template-event-forms branch from 4eba41b to cd1e89a Compare March 30, 2022 10:57
@GSadee
Copy link
Member

GSadee commented Mar 31, 2022

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "sylius-template-event-forms" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

@GSadee GSadee changed the base branch from master to 1.10 March 31, 2022 06:03
@GSadee GSadee force-pushed the sylius-template-event-forms branch from cd5f982 to f2a5ffa Compare March 31, 2022 06:03
content:
template: '@SyliusAdmin/AdminUser/Form/_form.html.twig'
priority: 10
sylius.admin.admin_user.first_column:
Copy link
Contributor

@vvasiloi vvasiloi Mar 31, 2022

Choose a reason for hiding this comment

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

Suggested change
sylius.admin.admin_user.first_column:
sylius.admin.admin_user.form.first_column:

Not including form might create confusion or even name collision in the future.

@GSadee GSadee merged commit 640ecbe into Sylius:1.10 Mar 31, 2022
@GSadee
Copy link
Member

GSadee commented Mar 31, 2022

Thank you, @SirDomin! 🥇

GSadee added a commit that referenced this pull request Apr 1, 2022
…adee)

This PR was merged into the 1.10 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.10
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | after #13798, replaces #13815
| License         | MIT

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

6423a5a [Admin] Move template events configs to a separate file
7ae36f7 [Admin] Clean up and refactor new template events
GSadee added a commit that referenced this pull request Apr 14, 2022
…nts (ernestWarwas)

This PR was merged into the 1.10 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.10
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | partially #13798
| License         | MIT

A new event for variants content and for additional information in an admin user form. 

<img width="709" alt="image" src="https://user-images.githubusercontent.com/28228691/163188527-77da6d4d-1d3a-4fa0-b470-204a0333e4f3.png">
⬇️ 

<img width="711" alt="image" src="https://user-images.githubusercontent.com/28228691/163188197-125d5d7f-4767-4e69-9f97-fc536b8325c7.png">


<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

127ec0f [Template][Events] new event and fix avatar in additional information
aba9df4 priority added to variatns content
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. DX Issues and PRs aimed at improving Developer eXperience.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants