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

Limit the max height/width of icons in the profiler menu #17330

Closed
wants to merge 3 commits into from

Conversation

javiereguiluz
Copy link
Member

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

@Nyholm
Copy link
Member

Nyholm commented Jan 11, 2016

Thank you @javiereguiluz You are awesome.

I can confirm that this resolves the issue on the profiler page.

This does not however address the issue in the toolbar. The "q." is outside the of the box. See attached screen shots

screen shot 2016-01-11 at 10 19 40
screen shot 2016-01-11 at 12 15 14

@javiereguiluz
Copy link
Member Author

Regarding the pending bug, is your HTML code similar to this?

<div class="sf-toolbar-block">
    <a href="...">
        <div class="sf-toolbar-icon">
            <svg ...></svg>

            <span class="sf-toolbar-value">1</span>
            <span class="sf-toolbar-label">req</span>
        </div>
    </a>

    <div class="sf-toolbar-info">
        ...
    </div>
</div>

Because I cannot reproduce the bug:

debug_toolbar

@Nyholm
Copy link
Member

Nyholm commented Jan 11, 2016

Yes, my HTML was similar to that.

With HTML copied from the documentation:

{% extends '@WebProfiler/Profiler/layout.html.twig' %}

{% import _self as macro %}

{% block toolbar %}
    {% if collector.totalRequests > 0 %}
        {% set icon %}
            {{ include('@Httplug/Icon/httplug.svg') }}
            <span class="sf-toolbar-status">Request</span>
        {% endset %}

        {% include 'WebProfilerBundle:Profiler:toolbar_item.html.twig' with { link: false } %}
    {% endif %}
{% endblock %}

screen shot 2016-01-11 at 12 37 52

@javiereguiluz
Copy link
Member Author

We don't use sf-toolbar-status in the toolbar for the built-in Symfony panels, but I did some quick test and it should work:

debug_toolbar_status

In some bundle I use the .*-status class and it also works as expected:

debug_status

This is very strange.

@Nyholm
Copy link
Member

Nyholm commented Jan 11, 2016

I think I made some progress. I removed my bundle from the equation and started with the twig icon.

The twig.svg file looks like this:

<svg version="1.1" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" height="24" viewBox="0 0 24 24" enable-background="new 0 0 24 24" xml:space="preserve">
<path fill="#AAAAAA" d="M20.1,1H3.9C2.3,1,1,2.3,1,3.9v16.3C1,21.7,2.3,23,3.9,23h16.3c1.6,0,2.9-1.3,2.9-2.9V3.9
    C23,2.3,21.7,1,20.1,1z M21,20.1c0,0.5-0.4,0.9-0.9,0.9H3.9C3.4,21,3,20.6,3,20.1V3.9C3,3.4,3.4,3,3.9,3h16.3C20.6,3,21,3.4,21,3.9
    V20.1z M5,5h14v3H5V5z M5,10h3v9H5V10z M10,10h9v9h-9V10z"/>
</svg>

And renders as we expect.

If I edit the twig.svg by removing height="24" so it looks like this:

<svg version="1.1" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" viewBox="0 0 24 24" enable-background="new 0 0 24 24" xml:space="preserve">
<path fill="#AAAAAA" d="M20.1,1H3.9C2.3,1,1,2.3,1,3.9v16.3C1,21.7,2.3,23,3.9,23h16.3c1.6,0,2.9-1.3,2.9-2.9V3.9
    C23,2.3,21.7,1,20.1,1z M21,20.1c0,0.5-0.4,0.9-0.9,0.9H3.9C3.4,21,3,20.6,3,20.1V3.9C3,3.4,3.4,3,3.9,3h16.3C20.6,3,21,3.4,21,3.9
    V20.1z M5,5h14v3H5V5z M5,10h3v9H5V10z M10,10h9v9h-9V10z"/>
</svg>

Then we will see the bug.

screen shot 2016-01-11 at 16 12 24

@Nyholm
Copy link
Member

Nyholm commented Jan 11, 2016

This will solve the issue:

.sf-toolbar-block .sf-toolbar-icon img,
.sf-toolbar-block .sf-toolbar-icon svg {
    height: 20px;
}

But I'm not sure we should use a fix height.

@javiereguiluz
Copy link
Member Author

Sorry for the delay. After several tests, I think @Nyholm's solution is the best one. Thanks for sharing it!

I did these changes:

.sf-toolbarreset svg,
.sf-toolbarreset img {
-    max-height: 20px;
-    max-width: 20px;
+    height: 20px;
}

#menu-profiler .label .icon img,
#menu-profiler .label .icon svg {
-    max-height: 24px;
+    height: 24px;
     max-width: 24px;
}

@Nyholm
Copy link
Member

Nyholm commented Jan 20, 2016

Thank you for getting back.

You all probably know this but I just want to highlight this. The sizes of the icons in the toolbar has changed from 2.7 to 2.8. Which means that this fix should not be merged "as is" to 2.7.

Symfony 2.7 uses 28px toolbar icons if i remember correctly.

@stof
Copy link
Member

stof commented Jan 20, 2016

@Nyholm this PR does not target Symfony 2.7 at all currently

@Nyholm
Copy link
Member

Nyholm commented Jan 20, 2016

@Nyholm this PR does not target Symfony 2.7 at all currently

Yeah, I know. My point was to highlight the fact that this PR should not be merged to 2.7

@fabpot
Copy link
Member

fabpot commented Jan 25, 2016

Thank you @javiereguiluz.

@fabpot fabpot closed this Jan 25, 2016
fabpot added a commit that referenced this pull request Jan 25, 2016
…javiereguiluz)

This PR was squashed before being merged into the 2.8 branch (closes #17330).

Discussion
----------

Limit the max height/width of icons in the profiler menu

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

Commits
-------

1f5f81c Limit the max height/width of icons in the profiler menu
@fabpot fabpot mentioned this pull request Feb 3, 2016
@fabpot fabpot mentioned this pull request Feb 28, 2016
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
… menu (javiereguiluz)

This PR was squashed before being merged into the 2.8 branch (closes symfony#17330).

Discussion
----------

Limit the max height/width of icons in the profiler menu

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

Commits
-------

1f5f81c Limit the max height/width of icons in the profiler menu
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.

None yet

6 participants