-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Webprofiler] Added blocks that allows extension of the profiler page. #23680
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never do this in profilers panel ... but I can't think of a good reason why we should do it. This way other bundles can improve profiler panels easily without adding new panels.
@@ -108,7 +110,9 @@ | |||
<p>None of the used translation messages are defined for the given locale.</p> | |||
</div> | |||
{% else %} | |||
{{ helper.render_table(messages_defined) }} | |||
{% block definedMessagesTable %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table
is an implementation detail (maybe some day we use two tables or a list, ...) so let's remove it from the block name. And let's use the common case used in Twig: {% block defined_messages %}
@@ -127,7 +131,9 @@ | |||
<p>No fallback translation messages were used.</p> | |||
</div> | |||
{% else %} | |||
{{ helper.render_table(messages_fallback) }} | |||
{% block fallbackMessagesTable %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{% block fallback_messages %}
@@ -147,11 +153,16 @@ | |||
<p>There are no messages of this category.</p> | |||
</div> | |||
{% else %} | |||
{{ helper.render_table(messages_missing) }} | |||
{% block missingMessagesTable %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{% block missing_messages %}
@@ -82,6 +82,8 @@ | |||
|
|||
<h2>Translation Messages</h2> | |||
|
|||
{% block panelContentHead %}{% endblock %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this block name because the panel content starts just after {% block panelContent %}
, so this can't be the head of the panel content. What if we rename {% block panelContentHead %} as {% block before_translation_messages %}
and {% block panelContentFooter %}
as {% block after_translation_messages %}
Thank you for the review. I've updated the PR |
a314fe5
to
d146727
Compare
Ive updated the PR to target 3.4. |
@@ -82,6 +82,8 @@ | |||
|
|||
<h2>Translation Messages</h2> | |||
|
|||
{% block before_translation_messages %}{% endblock %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of before
/after
blocks, I would prefer one block that wraps all the content, which can then be overridden easily with:
{% block contents %}
before
{{ parent() }}
after
{% endblock %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion. Thank you
@@ -82,6 +82,8 @@ | |||
|
|||
<h2>Translation Messages</h2> | |||
|
|||
{% block translation_messages %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are in a template managing templates, what about just messages
? Would that be clear enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Inside we have blocks named "missing_messages", "fallback_messages", and "defined_messages". So naming "translation_messages" to "messages" makes sense.
Thank you @Nyholm. |
…e profiler page. (Nyholm) This PR was squashed before being merged into the 3.4 branch (closes #23680). Discussion ---------- [Webprofiler] Added blocks that allows extension of the profiler page. | Q | A | ------------- | --- | Branch? | 3.4 (to be retargeted) | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Im not sure if we allow this but I thought I would try to submit this PR. The [TranslationBundle](https://github.com/php-translation/symfony-bundle) is extending the Symfony profiler page for translation. It would be great if it didn't have to replace all the contents but just replace some smaller blocks. Commits ------- ffd25fb [Webprofiler] Added blocks that allows extension of the profiler page.
Thank you for the review and for merging. |
…n of the profiler page. (Nyholm) This PR was squashed before being merged into the 3.4 branch (closes symfony#23680). Discussion ---------- [Webprofiler] Added blocks that allows extension of the profiler page. | Q | A | ------------- | --- | Branch? | 3.4 (to be retargeted) | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Im not sure if we allow this but I thought I would try to submit this PR. The [TranslationBundle](https://github.com/php-translation/symfony-bundle) is extending the Symfony profiler page for translation. It would be great if it didn't have to replace all the contents but just replace some smaller blocks. Commits ------- ffd25fb [Webprofiler] Added blocks that allows extension of the profiler page.
Im not sure if we allow this but I thought I would try to submit this PR.
The TranslationBundle is extending the Symfony profiler page for translation. It would be great if it didn't have to replace all the contents but just replace some smaller blocks.