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

Adjust the implementation to the new Twig Hooks version #16173

Conversation

jakubtobiasz
Copy link
Contributor

No description provided.

@probot-autolabeler probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Apr 23, 2024
Copy link

github-actions bot commented Apr 23, 2024

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

@jakubtobiasz jakubtobiasz force-pushed the bap/maintenance/twig-hooks-02 branch 2 times, most recently from 66aea6d to de29f05 Compare April 29, 2024 09:05
@jakubtobiasz jakubtobiasz force-pushed the bap/maintenance/twig-hooks-02 branch from de29f05 to 563b1db Compare April 29, 2024 09:09
@jakubtobiasz jakubtobiasz force-pushed the bap/maintenance/twig-hooks-02 branch from 775e35d to 30119a5 Compare April 29, 2024 14:31
@jakubtobiasz jakubtobiasz force-pushed the bap/maintenance/twig-hooks-02 branch from 30119a5 to f25a8bd Compare April 29, 2024 15:03
@jakubtobiasz jakubtobiasz marked this pull request as ready for review April 29, 2024 15:31
@jakubtobiasz jakubtobiasz requested review from a team as code owners April 29, 2024 15:31
parentMainHook: '@=parent_main_hook'
parentFallbackHook: '@=parent_fallback_hook'
props:
channel: '@=_context.resource'
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add the way of passing the resource to components/templates to our convention, now it's either the resource name or simply resource

Copy link
Member

Choose a reason for hiding this comment

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

by the way the we will stick out to the resource name rather than simply resource ?

Copy link
Contributor

Choose a reason for hiding this comment

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

With templates in mind we could just add both, that's basically in line with how those variables were in the templates before.
With components, it makes meagre sense since it needs to be added as a live prop (probably will be ignored otherwise, though I have not checked), in that case, I'd go w/ the resource name (stuff like public ?Product $resource = null; etc. imo looks weird).

Either way we gotta talk it through and decide what's the better option.

props:
form: '@=_context.form'
resource: '@=_context.resource'
isSimple: '@=_context.resource.isSimple()'
Copy link
Contributor

Choose a reason for hiding this comment

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

Having the property accessor take care of these expressions would be great, then we could go

@=context.resource.isSimple

or just

@=context.resource.simple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to do it in Twig Hooks v0.3.

Comment on lines +4 to +8
sylius_test_form_attribute('update-changes-button')|merge({
text: 'sylius.ui.update'|trans,
type: 'submit',
form: hookable_metadata.context.form.vars.id
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We really should reverse this merge, it looks weird having test attributes as the base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we should a better way to add test form attributes :D.

@@ -19,7 +16,7 @@
<div class="page-body">
<div class="container-xl">
{% block content %}
{% set renderRest = hookable_configuration.render_rest %}
{% set renderRest = hookable_metadata.configuration.render_rest %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add a global switch in the configuration regarding render_rest?

@jakubtobiasz jakubtobiasz force-pushed the bap/maintenance/twig-hooks-02 branch from 26fda4a to 60319eb Compare April 30, 2024 13:45
@jakubtobiasz jakubtobiasz force-pushed the bap/maintenance/twig-hooks-02 branch from 60319eb to d627142 Compare April 30, 2024 13:46
@NoResponseMate NoResponseMate merged commit 062fb9d into Sylius:bootstrap-admin-panel Apr 30, 2024
19 checks passed
@NoResponseMate
Copy link
Contributor

Thank you, @jakubtobiasz!

@jakubtobiasz jakubtobiasz deleted the bap/maintenance/twig-hooks-02 branch April 30, 2024 14:06
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants