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

[Webprofiler] Added blocks that allows extension of the profiler page. #23680

Closed
wants to merge 4 commits into from

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Jul 26, 2017

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 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.

Copy link
Member

@javiereguiluz javiereguiluz left a 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 %}
Copy link
Member

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 %}
Copy link
Member

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 %}
Copy link
Member

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 %}
Copy link
Member

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 %}

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 26, 2017
@Nyholm
Copy link
Member Author

Nyholm commented Jul 28, 2017

Thank you for the review. I've updated the PR

@Nyholm Nyholm changed the base branch from master to 3.4 July 29, 2017 15:37
@Nyholm
Copy link
Member Author

Nyholm commented Jul 29, 2017

Ive updated the PR to target 3.4.

@@ -82,6 +82,8 @@

<h2>Translation Messages</h2>

{% block before_translation_messages %}{% endblock %}
Copy link
Member

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 %}

Copy link
Member Author

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 %}
Copy link
Member

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?

Copy link
Member Author

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.

@fabpot
Copy link
Member

fabpot commented Aug 22, 2017

Thank you @Nyholm.

fabpot added a commit that referenced this pull request Aug 22, 2017
…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.
@fabpot fabpot closed this Aug 22, 2017
@Nyholm
Copy link
Member Author

Nyholm commented Aug 22, 2017

Thank you for the review and for merging.

@Nyholm Nyholm deleted the translation-table branch August 22, 2017 15:58
This was referenced Oct 18, 2017
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants