Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Limit the max height/width of icons in the profiler menu#17330
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
javiereguiluz commentedJan 11, 2016
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #17329 |
| License | MIT |
| Doc PR | - |
Nyholm commentedJan 11, 2016
Thank you@javiereguiluz You are awesome. I can confirm that this resolves the issue on the profiler page. This doesnot however address the issue in the toolbar. The "q." is outside the of the box. See attached screen shots
|
javiereguiluz commentedJan 11, 2016
Regarding the pending bug, is your HTML code similar to this? <divclass="sf-toolbar-block"><ahref="..."><divclass="sf-toolbar-icon"><svg...></svg><spanclass="sf-toolbar-value">1</span><spanclass="sf-toolbar-label">req</span></div></a><divclass="sf-toolbar-info"> ...</div></div> Because I cannot reproduce the bug:
|
Nyholm commentedJan 11, 2016
Yes, my HTML was similar to that. With HTML copied from the documentation: {%extends'@WebProfiler/Profiler/layout.html.twig' %}{%import_selfasmacro %}{%blocktoolbar %} {%ifcollector.totalRequests>0 %} {%seticon %} {{include('@Httplug/Icon/httplug.svg') }} <spanclass="sf-toolbar-status">Request</span> {%endset %} {%include'WebProfilerBundle:Profiler:toolbar_item.html.twig'with {link:false } %} {%endif %}{%endblock %}
|
javiereguiluz commentedJan 11, 2016
We don't use
In some bundle I use the
This is very strange. |
Nyholm commentedJan 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: <svgversion="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"><pathfill="#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 <svgversion="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"><pathfill="#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.
|
Nyholm commentedJan 11, 2016
This will solve the issue: .sf-toolbar-block .sf-toolbar-iconimg,.sf-toolbar-block .sf-toolbar-iconsvg {height:20px;} But I'm not sure we should use a fix height. |
javiereguiluz commentedJan 20, 2016
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 commentedJan 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 commentedJan 20, 2016
@Nyholm this PR does not target Symfony 2.7 at all currently |
Nyholm commentedJan 20, 2016
Yeah, I know. My point was to highlight the fact that this PR should not be merged to 2.7 |
fabpot commentedJan 25, 2016
Thank you@javiereguiluz. |
…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
… menu (javiereguiluz)This PR was squashed before being merged into the 2.8 branch (closessymfony#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