8000 [VarDumper] Minor fix for the uniformity by SofHad · Pull Request #13090 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[VarDumper] Minor fix for the uniformity #13090

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[VarDumper] Minor fix for the uniformity #13090

wants to merge 4 commits into from

Conversation

SofHad
Copy link
Contributor
@SofHad SofHad commented Dec 23, 2014
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets no
License MIT
Doc PR no

wouterj
wouterj reviewed Dec 23, 2014
View reviewed changes 8000
src/Symfony/Bundle/DebugBundle/Resources/views/Profiler/dump.html.twig
@@ -91,6 +91,10 @@

{{ dump.data|raw }}
</li>
{% else %}
<p>
Copy link
Member

Choose a reason for hiding this comment

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

this should be <li> and the indent should be one level back.

@SofHad
Copy link
Contributor Author
SofHad commented Dec 23, 2014

sure enough :)

@TomasVotruba
Copy link
Contributor

Just looking at the template, wouldn't it be better to apply same rule for count as above? And ff some, display the content?

{% if dumps_count %}

@fabpot
Copy link
Member
fabpot commented Dec 24, 2014

ping @nicolas-grekas

nicolas-grekas
nicolas-grekas reviewed Dec 24, 2014
View reviewed changes
src/Symfony/Bundle/DebugBundle/Resources/views/Profiler/dump.html.twig
@@ -91,6 +91,10 @@

{{ dump.data|raw }}
</li>
{% else %}
<li>
<em>No variables to dump</em>
Copy link
Member

Choose a reason for hiding this comment

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

No dumped variable

@nicolas-grekas
Copy link
Member

Can you add a screenshot please?

@SofHad
Copy link
Contributor Author
SofHad commented Dec 24, 2014

In order to ensure consistency throughout the WebProfiler it should to use p instead of li because the font-size is not the same. (li => 13px, p => 14px)

Here is the version with p

@nicolas-grekas Here is the screenshot:
for p
screenshot-127 0 0 1 8000 2014-12-24 11-50-01

for li
screenshot-127 0 0 1 8000 2014-12-24 12-14-13

@wouterj
Copy link
Member
wouterj commented Dec 24, 2014

In order to ensure consistency throughout 8000 the WebProfiler it should to use p instead of li because the font-size is not the same. (li => 13px, p => 14px)

Yeah, but that's invalid HTML as you cannot use a p element inside an ul element.

Instead, you should do something like:

     <ul class="alt">
+        {% if collection.dumpCount %}
-        {% for dump in collector.getDumps('html') %}
-        <li class="sf-dump sf-reset">
-            in
-            {% if dump.line %}
-                {% set link = dump.file|file_link(dump.line) %}
-                {% if link %}
-                    <a href="{{ link }}" title="{{ dump.file }}">{{ dump.name }}</a>
-                {% else %}
-                    <abbr title="{{ dump.file }}">{{ dump.name }}</abbr>
-                {% endif %}
-            {% else %}
-                {{ dump.name }}
-            {% endif %}
-            line {{ dump.line }}:
-            <a onclick="Sfdump.toggle(this)">▶</a>
-            <span class="sf-dump-compact">
-            {% if dump.fileExcerpt %}{{ dump.fileExcerpt|raw }}{% else %}{{ dump.file|file_excerpt(dump.line) }}{% endif %}
-            </span>
-
-            {{ dump.data|raw }}
-        </li>
-        {% endfor %}
+            {% for dump in collector.getDumps('html') %}
+            <li class="sf-dump sf-reset">
+                in
+                {% if dump.line %}
+                    {% set link = dump.file|file_link(dump.line) %}
+                    {% if link %}
+                        <a href="{{ link }}" title="{{ dump.file }}">{{ dump.name }}</a>
+                    {% else %}
+                        <abbr title="{{ dump.file }}">{{ dump.name }}</abbr>
+                    {% endif %}
+                {% else %}
+                    {{ dump.name }}
+                {% endif %}
+                line {{ dump.line }}:
+                <a onclick="Sfdump.toggle(this)">▶</a>
+                <span class="sf-dump-compact">
+                {% if dump.fileExcerpt %}{{ dump.fileExcerpt|raw }}{% else %}{{ dump.file|file_excerpt(dump.line) }}{% endif %}
+                </span>
+
+                {{ dump.data|raw }}
+            </li>
+            {% endfor %}
+        {% else %}
+        <p>No dumped variable</p>
+        {% endif %}
     </ul>

@SofHad
Copy link
Contributor Author
SofHad commented Dec 24, 2014

@wouterj
Indeed, I've updated here
Thank you

{% if collector.dumpsCount %}
        <ul class="alt">
            {% for dump in collector.getDumps('html') %}
            <li class="sf-dump sf-reset">
                in
                {% if dump.line %}
                    {% set link = dump.file|file_link(dump.line) %}
                    {% if link %}
                        <a href="{{ link }}" title="{{ dump.file }}">{{ dump.name }}</a>
                    {% else %}
                        <abbr title="{{ dump.file }}">{{ dump.name }}</abbr>
                    {% endif %}
                {% else %}
                    {{ dump.name }}
                {% endif %}
                line {{ dump.line }}:
                <a onclick="Sfdump.toggle(this)">▶</a>
                <span class="sf-dump-compact">
                {% if dump.fileExcerpt %}{{ dump.fileExcerpt|raw }}{% else %}{{ dump.file|file_excerpt(dump.line) }}{% endif %}
                </span>

                {{ dump.data|raw }}
            </li>
            {% endfor %}
        </ul>
    {% else %}
        <p>
            <em>No dumped variable</em>
        </p>
    {% endif %}

@wouterj
Copy link
Member
wouterj commented Dec 24, 2014

@SofHad you should commit directly into this branch, the PR will be updated automatically.

Btw, I think "No dumped data" or "No dumped variables" is better.

@SofHad
Copy link
Contributor Author
SofHad commented Dec 24, 2014

Sorry, It is done now :)

@stof
Copy link
Member
stof commented Dec 24, 2014

👍

1 similar comment
@nicolas-grekas
Copy link
Member

👍

@nicolas-grekas
Copy link
Member

Thank you @SofHad.

nicolas-grekas added a commit that referenced this pull request Dec 24, 2014
@nicolas-grekas
minor #13090 [VarDumper] Minor fix for the uniformity (SofHad)
bad5169 
This PR was submitted for the master branch but it was merged into the 2.6 branch instead (closes #13090).

Discussion
----------

[VarDumper] Minor fix for the uniformity

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | no
| License       | MIT
| Doc PR        | no

Commits
-------

7d02e48 [VarDumper] Minor fix for the uniformity
@nicolas-grekas
Copy link
Member

Closed via bad5169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SofHad @TomasVotruba @fabpot @nicolas-grekas @wouterj @stof
0