Re-design the debugging toolbar #3850
Conversation
|
@shiroyuki your testing app denies the access because of the restriction in app_dev.php |
|
@stof: I'm sorry. It should be working now. |
| <span> | ||
| <img width="26" height="28" alt="PHP" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABoAAAAcCAYAAAB/E6/TAAAD8GlDQ1BJQ0MgUHJvZmlsZQAAKJGNVd1v21QUP4lvXKQWP6Cxjg4Vi69VU1u5GxqtxgZJk6XpQhq5zdgqpMl1bhpT1za2021Vn/YCbwz4A4CyBx6QeEIaDMT2su0BtElTQRXVJKQ9dNpAaJP2gqpwrq9Tu13GuJGvfznndz7v0TVAx1ea45hJGWDe8l01n5GPn5iWO1YhCc9BJ/RAp6Z7TrpcLgIuxoVH1sNfIcHeNwfa6/9zdVappwMknkJsVz19HvFpgJSpO64PIN5G+fAp30Hc8TziHS4miFhheJbjLMMzHB8POFPqKGKWi6TXtSriJcT9MzH5bAzzHIK1I08t6hq6zHpRdu2aYdJYuk9Q/881bzZa8Xrx6fLmJo/iu4/VXnfH1BB/rmu5ScQvI77m+BkmfxXxvcZcJY14L0DymZp7pML5yTcW61PvIN6JuGr4halQvmjNlCa4bXJ5zj6qhpxrujeKPYMXEd+q00KR5yNAlWZzrF+Ie+uNsdC/MO4tTOZafhbroyXuR3Df08bLiHsQf+ja6gTPWVimZl7l/oUrjl8OcxDWLbNU5D6JRL2gxkDu16fGuC054OMhclsyXTOOFEL+kmMGs4i5kfNuQ62EnBuam8tzP+Q+tSqhz9SuqpZlvR1EfBiOJTSgYMMM7jpYsAEyqJCHDL4dcFFTAwNMlFDUUpQYiadhDmXteeWAw3HEmA2s15k1RmnP4RHuhBybdBOF7MfnICmSQ2SYjIBM3iRvkcMki9IRcnDTthyLz2Ld2fTzPjTQK+Mdg8y5nkZfFO+se9LQr3/09xZr+5GcaSufeAfAww60mAPx+q8u/bAr8rFCLrx7s+vqEkw8qb+p26n11Aruq6m1iJH6PbWGv1VIY25mkNE8PkaQhxfLIF7DZXx80HD/A3l2jLclYs061xNpWCfoB6WHJTjbH0mV35Q/lRXlC+W8cndbl9t2SfhU+Fb4UfhO+F74GWThknBZ+Em4InwjXIyd1ePnY/Psg3pb1TJNu15TMKWMtFt6ScpKL0ivSMXIn9QtDUlj0h7U7N48t3i8eC0GnMC91dX2sTivgloDTgUVeEGHLTizbf5Da9JLhkhh29QOs1luMcScmBXTIIt7xRFxSBxnuJWfuAd1I7jntkyd/pgKaIwVr3MgmDo2q8x6IdB5QH162mcX7ajtnHGN2bov71OU1+U0fqqoXLD0wX5ZM005UHmySz3qLtDqILDvIL+iH6jB9y2x83ok898GOPQX3lk3Itl0A+BrD6D7tUjWh3fis58BXDigN9yF8M5PJH4B8Gr79/F/XRm8m241mw/wvur4BGDj42bzn+Vmc+NL9L8GcMn8F1kAcXjEKMJAAAAACXBIWXMAAAsTAAALEwEAmpwYAAABbmlUWHRYTUw6Y29tLmFkb2JlLnhtcAAAAAAAPHg6eG1wbWV0YSB4bWxuczp4PSJhZG9iZTpuczptZXRhLyIgeDp4bXB0az0iWE1QIENvcmUgNC40LjAiPgogICA8cmRmOlJERiB4bWxuczpyZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1ucyMiPgogICAgICA8cmRmOkRlc2NyaXB0aW9uIHJkZjphYm91dD0iIgogICAgICAgICAgICB4bWxuczpkYz0iaHR0cDovL3B1cmwub3JnL2RjL2VsZW1lbnRzLzEuMS8iPgogICAgICAgICA8ZGM6c3ViamVjdD4KICAgICAgICAgICAgPHJkZjpCYWcvPgogICAgICAgICA8L2RjOnN1YmplY3Q+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpSREY+CjwveDp4bXBtZXRhPgrlPw1BAAADrElEQVRIicWWzUtjVxjGf+d6QRFBIkh0wMbcwAhKbsChC3fa0rgRHPAPMBtBzaYutLgruKqrrvwDxK0wWQmVjoq4UUdUFI2QyA1aQcH6Gb9O7tuFiZMZxxjBoQ/czTmc53c+uO/zKhGhkAKBwFvXdd8rpX4BflBKvQEQkX+AlIhMG4bxIZFI7BTyUU+BfD7fO9M0R4GfCu7ksz5qrYccx/lUFEgpZfr9/j+BfqWUKhICgNybje3u7v4qIvpJkM/n85imOQm0vQTwDc1orbscx/k3N2A8EJUyXwkC0Gaa5qRSynwEyl7Xa0AeYFlPIHt1Pp/vXUlJydJL3+Q5iYhkMpkfHcf5ZACYpjn62hAAdf8eowDK7/c3KKW2i11cV1dHOBxmcnKSk5OTYpc1GCLS+ZJdtre3Mzg4SDqdLnqN67rvSzwez+9KKSt/ora2lo6ODgA6Ozs5Pz/n+PgYgEgkwt3dHY7jEA6HOTw85OzsjFAoRHNzMzU1NbS1tbG3t8fV1VXOUuH3++OWZUn+NzIyIiIiy8vLMjs7KyIi4XBYLMuSVColV1dXEovF5ODgQBYXF8WyLJmenhattUxNTUk8HpfDw0MJBAJiWZb4/f64katd+bJtm6OjIyKRCD09PVxeXtLa2orH46Guro6JiQkGBgZYXFykvLwcgGAwyNzcHNFolOHhYaqrq2lqaro/jlJvjK8hAKFQiNXVVdLpNKZpUlZWhtaaUCgEQCwWA6CxsZGtrS28Xi9er5eFhQWAB3gmk8lZipGtwg+qrKzE5/PR1NSE1+slGo3iui4zMzPYts3l5SXb29tUVFQQCARYW1vDtm0AWlpaqKqqore3l2Qyyc7OfUEXkQMTSAFvc6BgMAjAysoK8/Pz3NzcMDw8jOM4hEIhNjY2cF2XYDCIUor19XXa29s5PT3FNE2WlpbY39+nr68v/0QpVV9fP2QYxh+5kWg0Sn9/P7ZtU1payu3tLVp/UYgfaXx8HBGhu7ubyspKTk9Pv5h3Xfc3QykVyx+0bZvNzU0ymQzpdPpZiFKKYDDI2toawCMIgGEYH5SIEAgE/iYbcOXl5biuy/X1dUFAvioqKri+vn5qUx8TicTPBoDWeigbWqTT6RdBAC4uLr4JERHRWg9BNiay8Tv2IvfiNJaL9oeEVUqZlmX9xetl0kwymQznIv3hh70/pe4CZl4DorXuyu8b/p/mJF/fvd36WpZlNYhIZ6EGUikVSyaT8UI+z4JeS9+s3t9D/wE16uReKJ4y+wAAAABJRU5ErkJggg=="/> | ||
| </span> | ||
| {% endset %} |
There was a problem hiding this comment.
is it intended to change the icon only in verbose mode ?
There was a problem hiding this comment.
No, I forgot to remove the verbose. :P
… is on the top of the page.
|
Moving the toolbar to the top of the page means it will hide some content of the page. You should keep it at the bottom |
|
Just a moment ago, I changed the position of the toolbar via I just reverted the config file. :D |
|
Some comments:
But overall, this is a very nice improvement. |
|
There is an issue with "bubbling" at Firefox 11 (at least), when you hover |
|
As the verbose mode has been removed from the template, it should also be removed from the configuration (I can do that after merging if you don't know how to do that). |
|
Will there be options to somehow keep the debug toolbar 'expanded' or something? I guess the folding of the sf and php information makes sense, but I personally look at the request/time/memory/security and query parts of the toolbar a lot. As my browser window is big enough to show all information at once, this would be a huge step backwards imo. |
|
Agreed with @asm89, I also want the option to show all the information on my screen. |
|
I tend to agree too with @asm89. What about reusing the |
|
How about using media query? |
…rity (both a abbreviation and an associate description) and number of DB requests and request time.
|
Please note that the latest commit still doesn't have the new logo for PHP. As DoctrineBundle now has its own repository, the change to show the number of DB requests is already done via DoctrineBundle's PR 57. |
|
@fabpot @shiroyuki as soon as this patch is merged I will do the same on DoctrineBundle. |
|
The last commit has the updated PHP logo. Unfortunately as @stloyd and @guilhermeblanco pointed out, the flicking on the toolbar when the mouse is over might have been due to the CSS issue on Firefox. |
|
Nice work shiroyuki. I always had the feeling the toolbar can be improved. Good that you got this one going. |
|
Another idea: Add a panel "PHP Info" to the profiler that shows the output of |
|
@Tobion: It would be an overkill if Please note that the media query is not yet implement. The followings are still unknown to me:
|
|
@shiroyuki you misunderstood me. phpinfo() should be a new panel in the PROFILER, not the WDT. It is reachable from the WDT by clicking on the PHP logo. But that can be implemented in a seperate PR. It's just an idea and before I would implement it, I'd like to receive feedback if it would be accepted at all. |
|
Displaying |
|
@fabpot yeah. But would you accept such a PR or do you think it's not useful? |
|
@Tobion The web profiler is mainly about information for the current request; so I'm not sure it would be useful to have such a tab in the profiler. |
|
The equivalent for 👍 for that! |
Good question, I don't think so because the screen-size is to small to show anything useful. |
|
If your interested in a useful but not so intrusive way of providing the toolbar on mobile devices perhaps take a look at what the guys at Twitter have done with the topbar navigation converting to a semi-accordion style menu on mobile in Bootstrap. I can see the usefulness. Checkout the responsive.less which makes it easy enough to include/exclude depending on screen size. I found it quite useful in a recent project. Regarding adding a tab for phpinfo, sure it would be useful BUT if the reasoning is that some people leave a publicly available phpinfo script therefore just include it then I would not include it. There are many more useful requirements of the toolbar rather than to insulate intro to web issues. That's like saying don't include the toolbar because someone may build an application that makes the toolbar available publicly (which will happen). I've seen too many projects in my years having no clue of versioning tools that must have been built on the server, live, with filenames like indexv1.php, indexv2.php, indexTryAgain.php, db credentials in the clear, and just hoping to find a point where it works enough. And yes, I've found those scripts were publicly available and still around years after they were created; security holes and all! I'm preaching to the choir here. You'll never stop stupid. All we can do is educate by any means we have and share our knowledge with one another. Aside from that devils advocate reasoning, I would include the phpinfo tab, it does make sense in those random "did I/they compile that in" circumstances. ;-) Sorry for the rant. +1 for mobile Again, sorry for the long winded rant. Cheers everyone. Goodnight. |
…ticated users and exception notification.
|
@stof I think we can remove the CSS. |
|
It is much better now... but still not perfect! Here is a screenshot of the WDT at different sizes (the first being he WDT in the profiler): Some comments:
|
|
@fabpot screenshot is broken (unreadable) |
|
(FYI to all, the machine is now online again. Sorry... I needed to bring it to work yesterday. XP ) @fabpot first of all, if "display all available information" meant "i don't see the information about doctrine", it would be because of the pull request for DoctrineBundle I submitted awhile ago. I forgot to mention that. Can you also tell me which tab is very large (I can't see from my environment) and what the bug in the profiler is? Also, you may notice that there is a free space on the left. Please note that it is left by choice as I need the doctrine tab/block to be fully visible within the viewport without resizing the window. The latest commit is for breaking hell... sorry i meant show all information. This is only available when the window is at least to 1440 pixel wide. |
| <span class="sf-toolbar-info-class sf-toolbar-info-with-next-pointer">{{ collector.controller.class|abbr_class }}</span> | ||
| <span class="sf-toolbar-info-method"> | ||
| {% if link %} | ||
| <a style="color: #2f2f2f" href="{{ link }}">{{ collector.controller.method }}</a> |
There was a problem hiding this comment.
This is always hidden; probably because of the a tag.
There was a problem hiding this comment.
Are you referring to when it is hidden due to the media query?
There was a problem hiding this comment.
Have a look at my screenshot, it is never displayed (this is probably due to a media query side-effect).
There was a problem hiding this comment.
@fabpot your screenshot does not work. The link gives us Not found
There was a problem hiding this comment.
seems to be http://twitpic.com/99jcvh/full
There was a problem hiding this comment.
this is weird. It does not look the same than on the demo. The demo shows the link properly
There was a problem hiding this comment.
@fabpot how do you test this? on your local machine?
There was a problem hiding this comment.
The difference with your demo is that you don't have the link, because you have not configured the IDE in your configuration. Force link to any value and you will see what I have on my computer.
|
I have two more suggestions: |
|
One more: I would rename the tooltip for the X-button from "Hide Toolbar" to "Close Toolbar". Hide implicitly says you have a chance to display it again, but you cannot without refreshing the page, which creates a new profiler token. |
|
@shiroyuki Just one note, PHP logo should use same background as Symfony one (now is a bit darker), also would be nice to make route name a bit more noticable (i.e. add: |
|
@stloyd why should the route name be more noticable than other information? |
|
I fixed the |
Commits ------- 75c7d3a Fixed the link to the method with onclick event. ecbabec renamed 'Request handler' to 'Controller' and 'Route ID' to 'Route name'. 3c5ede4 Add a new query to display all information. f6a866b Merged CSS for the toolbar in both embedded mode (on each page) and profiler. 306533b Updated the responsive design in addition to the scenario with authenticated users and exception notification. 4a3312b Updated the toolbar with the responsive design (normal-to-large scenario). 1eec2a2 Updated the toolbar with the responsive design (normal-to-small scenario). 03c8213 Refactored the CSS code for the toolbar out of the template. 37843b3 Updated with PHP logo (only the text). d5e0ccc Made the toolbar to show the version, memory usage, the state of security (both a abbreviation and an associate description) and number of DB requests and request time. 37ad8a6 Removed the check for verbose and adjusted the style when the toolbar is on the top of the page. 67b0532 Redesigned the WDT. Discussion ---------- Re-design the debugging toolbar The toolbar is very useful and containing lots of information. However, as there are too much information, it is very distracting and the toolbar area somehow ends up taking too much space and then becomes something like a panel. The main purpose of this pull request is to hide any information and show only whenever the user wants to see, except the status code and response time. This is based on [the pull request #3833](#3833) with the feedbacks and for 2.1 (master). The testing app is available at http://home.shiroyuki.com. --------------------------------------------------------------------------- by stof at 2012-04-10T06:24:36Z @shiroyuki your testing app denies the access because of the restriction in app_dev.php --------------------------------------------------------------------------- by shiroyuki at 2012-04-10T06:27:27Z @stof: I'm sorry. It should be working now. --------------------------------------------------------------------------- by stof at 2012-04-10T06:45:39Z Moving the toolbar to the top of the page means it will hide some content of the page. You should keep it at the bottom --------------------------------------------------------------------------- by shiroyuki at 2012-04-10T06:48:28Z Just a moment ago, I changed the position of the toolbar via `config_dev` so I could check when WDT is on the top. I just reverted the config file. :D --------------------------------------------------------------------------- by fabpot at 2012-04-10T06:55:16Z Some comments: * I would have kept the number of database request as this number is probably the one everybody should have a look at on every page. * I would have used the original PHP logo (in black and white) instead of a non-standard one But overall, this is a very nice improvement. --------------------------------------------------------------------------- by stloyd at 2012-04-10T06:55:43Z There is an issue with "bubbling" at Firefox 11 (at least), when you hover `<a>` element, the hover event seems to be "launched" twice. --------------------------------------------------------------------------- by fabpot at 2012-04-10T06:56:13Z As the verbose mode has been removed from the template, it should also be removed from the configuration (I can do that after merging if you don't know how to do that). --------------------------------------------------------------------------- by shiroyuki at 2012-04-10T07:05:31Z @stloyd I noticed that too. As I couldn't find the same issue on Webkit-based browsers and all effects on this toolbar heavily relies on CSS, it could have been a glitch on Firefox. @fabpot I'll see what I can do with the number of DB request and the logo. --------------------------------------------------------------------------- by asm89 at 2012-04-10T07:26:28Z Will there be options to somehow keep the debug toolbar 'expanded' or something? I guess the folding of the sf and php information makes sense, but I personally look at the request/time/memory/security and query parts of the toolbar a lot. As my browser window is big enough to show all information at once, this would be a huge step backwards imo. --------------------------------------------------------------------------- by XWB at 2012-04-10T07:28:38Z Agreed with @asm89, I also want the option to show all the information on my screen. --------------------------------------------------------------------------- by fabpot at 2012-04-10T08:28:00Z I tend to agree too with @asm89. What about reusing the `verbose` option for that. This was already its purpose anyway. --------------------------------------------------------------------------- by shiroyuki at 2012-04-10T14:56:45Z How about using media query? --------------------------------------------------------------------------- by shiroyuki at 2012-04-11T02:20:32Z Please note that the latest commit still doesn't have the new logo for PHP. As DoctrineBundle now has its own repository, the change to show the number of DB requests is already done via DoctrineBundle's [PR 57](doctrine/DoctrineBundle#57). --------------------------------------------------------------------------- by guilhermeblanco at 2012-04-11 F438 T02:50:47Z @fabpot @shiroyuki as soon as this patch is merged I will do the same on DoctrineBundle. All you need to do is look at me over our desks' separator. =D --------------------------------------------------------------------------- by shiroyuki at 2012-04-11T03:17:41Z The last commit has the updated PHP logo. Unfortunately as @stloyd and @guilhermeblanco pointed out, the flicking on the toolbar when the mouse is over might have been due to the CSS issue on Firefox. --------------------------------------------------------------------------- by Tobion at 2012-04-11T04:46:36Z Nice work shiroyuki. I always had the feeling the toolbar can be improved. Good that you got this one going. I would remove the verbose option (rarely nobody changes it) and use media queries to accomplish a responsive design that shows as much information as possible. And only shows the most important facts when there is not enough space. E.g. the symfony version could be removed if it doesn't fit on the screen because it's mostly static from request to request. --------------------------------------------------------------------------- by Tobion at 2012-04-11T04:48:45Z Another idea: Add a panel "PHP Info" to the profiler that shows the output of `phpinfo()`. This panel is linked from the PHP logo in the WDT which currently has no link on it. --------------------------------------------------------------------------- by shiroyuki at 2012-04-11T15:47:51Z @Tobion: It would be an overkill if `phpinfo()` was visible in the toolbar. Additionally, the toolbar doesn't fit to show that amount of information. Plus, the information released by `phpinfo()` is also static and easily obtained by a simple PHP script. I don't think that WDT should be showing this information. Please note that the media query is not yet implement. The followings are still unknown to me: * should we support the toolbar for mobile device? * what is the minimum screen size? --------------------------------------------------------------------------- by Tobion at 2012-04-11T15:52:43Z @shiroyuki you misunderstood me. phpinfo() should be a new panel in the PROFILER, not the WDT. It is reachable from the WDT by clicking on the PHP logo. But that can be implemented in a seperate PR. It's just an idea and before I would implement it, I'd like to receive feedback if it would be accepted at all. --------------------------------------------------------------------------- by fabpot at 2012-04-11T16:38:44Z Displaying `phpinfo()` data is not in the scope of this PR. --------------------------------------------------------------------------- by Tobion at 2012-04-11T16:48:50Z @fabpot yeah. But would you accept such a PR or do you think it's not useful? --------------------------------------------------------------------------- by fabpot at 2012-04-11T16:57:49Z @Tobion The web profiler is mainly about information for the current request; so I'm not sure it would be useful to have such a tab in the profiler. --------------------------------------------------------------------------- by vicb at 2012-04-11T17:06:15Z @fabpot @Tobion what about adding it in the config panel ? Not sure if it is very useful but I have seen to many `phpinfo.php` in the web root folder. (It could be an expandable panel loaded via ajax like what is used for the Doctrine explain panel). --------------------------------------------------------------------------- by shiroyuki at 2012-04-12T03:11:40Z @tobian @vicb: what kind of information are you looking from `phpinfo()`? --------------------------------------------------------------------------- by Felds at 2012-04-12T03:30:02Z The equivalent for `phpinfo()` was extremely convenient and helped a lot in Symfony 1. It's out of scope but an optional panel could be nice. Ini flags are of great help when debugging on a hurry. :+1: for that! --------------------------------------------------------------------------- by Tobion at 2012-04-12T03:37:52Z @shiroyuki I don't understand your question. Everything of it should be displayed. But don't worry about phpinfo(), I'll work on that in a seperate PR. You can focus on the responsive design. ;) --------------------------------------------------------------------------- by vicb at 2012-04-12T06:54:35Z @shiroyuki I am not looking for anything specific. Just saying I have seen many times customer code using a publicly accessible file to return the info and it would help to get ride of this file. --------------------------------------------------------------------------- by sstok at 2012-04-12T07:59:18Z ``` should we support the toolbar for mobile device? ``` Good question, I don't think so because the screen-size is to small to show anything useful. Maybe a small icon to display the information as overlay, including the token so you can refer to that on a bigger screen?. --------------------------------------------------------------------------- by johnnypeck at 2012-04-13T06:45:43Z If your interested in a useful but not so intrusive way of providing the toolbar on mobile devices perhaps take a look at what the guys at Twitter have done with the topbar navigation converting to a semi-accordion style menu on mobile in Bootstrap. I can see the usefulness. Checkout the responsive.less which makes it easy enough to include/exclude depending on screen size. I found it quite useful in a recent project. Regarding adding a tab for phpinfo, sure it would be useful BUT if the reasoning is that some people leave a publicly available phpinfo script therefore just include it then I would not include it. There are many more useful requirements of the toolbar rather than to insulate intro to web issues. That's like saying don't include the toolbar because someone may build an application that makes the toolbar available publicly (which will happen). I've seen too many projects in my years having no clue of versioning tools that must have been built on the server, live, with filenames like indexv1.php, indexv2.php, indexTryAgain.php, db credentials in the clear, and just hoping to find a point where it works enough. And yes, I've found those scripts were publicly available and still around years after they were created; security holes and all! I'm preaching to the choir here. You'll never stop stupid. All we can do is educate by any means we have and share our knowledge with one another. Aside from that devils advocate reasoning, I would include the phpinfo tab, it does make sense in those random "did I/they compile that in" circumstances. ;-) Sorry for the rant. +1 for mobile sorta+1 for phpinfo +10 for better educating on how to include anything you need so phpinfo could be a "my first foray into adding a tool to my toolbar for Symfony" tutorial in the cookbook. Again, sorry for the long winded rant. Cheers everyone. Goodnight. --------------------------------------------------------------------------- by shiroyuki at 2012-04-13T23:33:21Z @stof I think we can remove the CSS.
|
Thanks for this :) |
|
Really nice re-design, thank you! |
Commits ------- 9f772c7 [WDT] Ajusted the color of the PHP logo Discussion ---------- [WebProfilerBundle] Ajusted the color of the PHP logo Since the redesign of the Web-Debug-Toolbar in #3850, a new PHP icon has been added, but its color is a bit darker (`#000000`) than the other icons in the toolbar (`#302e32` for the Symfony2 logo). @stloyd also noticed it [here](#3850 (comment)). This (minor) PR aims to ajust the background color of the PHP logo to keep a certain homogeneity. The color change can be see there (before/after) : http://twitpic.com/9kbs4p --------------------------------------------------------------------------- by travisbot at 2012-05-12T21:03:31Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1316131) (merged 9f772c7 into e193452).
Commits ------- 75c7d3a Fixed the link to the method with onclick event. ecbabec renamed 'Request handler' to 'Controller' and 'Route ID' to 'Route name'. 3c5ede4 Add a new query to display all information. f6a866b Merged CSS for the toolbar in both embedded mode (on each page) and profiler. 306533b Updated the responsive design in addition to the scenario with authenticated users and exception notification. 4a3312b Updated the toolbar with the responsive design (normal-to-large scenario). 1eec2a2 Updated the toolbar with the responsive design (normal-to-small scenario). 03c8213 Refactored the CSS code for the toolbar out of the template. 37843b3 Updated with PHP logo (only the text). d5e0ccc Made the toolbar to show the version, memory usage, the state of security (both a abbreviation and an associate description) and number of DB requests and request time. 37ad8a6 Removed the check for verbose and adjusted the style when the toolbar is on the top of the page. 67b0532 Redesigned the WDT. Discussion ---------- Re-design the debugging toolbar The toolbar is very useful and containing lots of information. However, as there are too much information, it is very distracting and the toolbar area somehow ends up taking too much space and then becomes something like a panel. The main purpose of this pull request is to hide any information and show only whenever the user wants to see, except the status code and response time. This is based on [the pull request symfony#3833](symfony#3833) with the feedbacks and for 2.1 (master). The testing app is available at http://home.shiroyuki.com. --------------------------------------------------------------------------- by stof at 2012-04-10T06:24:36Z @shiroyuki your testing app denies the access because of the restriction in app_dev.php --------------------------------------------------------------------------- by shiroyuki at 2012-04-10T06:27:27Z @stof: I'm sorry. It should be working now. --------------------------------------------------------------------------- by stof at 2012-04-10T06:45:39Z Moving the toolbar to the top of the page means it will hide some content of the page. You should keep it at the bottom --------------------------------------------------------------------------- by shiroyuki at 2012-04-10T06:48:28Z Just a moment ago, I changed the position of the toolbar via `config_dev` so I could check when WDT is on the top. I just reverted the config file. :D --------------------------------------------------------------------------- by fabpot at 2012-04-10T06:55:16Z Some comments: * I would have kept the number of database request as this number is probably the one everybody should have a look at on every page. * I would have used the original PHP logo (in black and white) instead of a non-standard one But overall, this is a very nice improvement. --------------------------------------------------------------------------- by stloyd at 2012-04-10T06:55:43Z There is an issue with "bubbling" at Firefox 11 (at least), when you hover `<a>` element, the hover event seems to be "launched" twice. --------------------------------------------------------------------------- by fabpot at 2012-04-10T06:56:13Z As the verbose mode has been removed from the template, it should also be removed from the configuration (I can do that after merging if you don't know how to do that). --------------------------------------------------------------------------- by shiroyuki at 2012-04-10T07:05:31Z @stloyd I noticed that too. As I couldn't find the same issue on Webkit-based browsers and all effects on this toolbar heavily relies on CSS, it could have been a glitch on Firefox. @fabpot I'll see what I can do with the number of DB request and the logo. --------------------------------------------------------------------------- by asm89 at 2012-04-10T07:26:28Z Will there be options to somehow keep the debug toolbar 'expanded' or something? I guess the folding of the sf and php information makes sense, but I personally look at the request/time/memory/security and query parts of the toolbar a lot. As my browser window is big enough to show all information at once, this would be a huge step backwards imo. --------------------------------------------------------------------------- by XWB at 2012-04-10T07:28:38Z Agreed with @asm89, I also want the option to show all the information on my screen. --------------------------------------------------------------------------- by fabpot at 2012-04-10T08:28:00Z I tend to agree too with @asm89. What about reusing the `verbose` option for that. This was already its purpose anyway. --------------------------------------------------------------------------- by shiroyuki at 2012-04-10T14:56:45Z How about using media query? --------------------------------------------------------------------------- by shiroyuki at 2012-04-11T02:20:32Z Please note that the latest commit still doesn't have the new logo for PHP. As DoctrineBundle now has its own repository, the change to show the number of DB requests is already done via DoctrineBundle's [PR 57](doctrine/DoctrineBundle#57). --------------------------------------------------------------------------- by guilhermeblanco at 2012-04-11T02:50:47Z @fabpot @shiroyuki as soon as this patch is merged I will do the same on DoctrineBundle. All you need to do is look at me over our desks' separator. =D --------------------------------------------------------------------------- by shiroyuki at 2012-04-11T03:17:41Z The last commit has the updated PHP logo. Unfortunately as @stloyd and @guilhermeblanco pointed out, the flicking on the toolbar when the mouse is over might have been due to the CSS issue on Firefox. --------------------------------------------------------------------------- by Tobion at 2012-04-11T04:46:36Z Nice work shiroyuki. I always had the feeling the toolbar can be improved. Good that you got this one going. I would remove the verbose option (rarely nobody changes it) and use media queries to accomplish a responsive design that shows as much information as possible. And only shows the most important facts when there is not enough space. E.g. the symfony version could be removed if it doesn't fit on the screen because it's mostly static from request to request. --------------------------------------------------------------------------- by Tobion at 2012-04-11T04:48:45Z Another idea: Add a panel "PHP Info" to the profiler that shows the output of `phpinfo()`. This panel is linked from the PHP logo in the WDT which currently has no link on it. --------------------------------------------------------------------------- by shiroyuki at 2012-04-11T15:47:51Z @Tobion: It would be an overkill if `phpinfo()` was visible in the toolbar. Additionally, the toolbar doesn't fit to show that amount of information. Plus, the information released by `phpinfo()` is also static and easily obtained by a simple PHP script. I don't think that WDT should be showing this information. Please note that the media query is not yet implement. The followings are still unknown to me: * should we support the toolbar for mobile device? * what is the minimum screen size? --------------------------------------------------------------------------- by Tobion at 2012-04-11T15:52:43Z @shiroyuki you misunderstood me. phpinfo() should be a new panel in the PROFILER, not the WDT. It is reachable from the WDT by clicking on the PHP logo. But that can be implemented in a seperate PR. It's just an idea and before I would implement it, I'd like to receive feedback if it would be accepted at all. --------------------------------------------------------------------------- by fabpot at 2012-04-11T16:38:44Z Displaying `phpinfo()` data is not in the scope of this PR. --------------------------------------------------------------------------- by Tobion at 2012-04-11T16:48:50Z @fabpot yeah. But would you accept such a PR or do you think it's not useful? --------------------------------------------------------------------------- by fabpot at 2012-04-11T16:57:49Z @Tobion The web profiler is mainly about information for the current request; so I'm not sure it would be useful to have such a tab in the profiler. --------------------------------------------------------------------------- by vicb at 2012-04-11T17:06:15Z @fabpot @Tobion what about adding it in the config panel ? Not sure if it is very useful but I have seen to many `phpinfo.php` in the web root folder. (It could be an expandable panel loaded via ajax like what is used for the Doctrine explain panel). --------------------------------------------------------------------------- by shiroyuki at 2012-04-12T03:11:40Z @tobian @vicb: what kind of information are you looking from `phpinfo()`? --------------------------------------------------------------------------- by Felds at 2012-04-12T03:30:02Z The equivalent for `phpinfo()` was extremely convenient and helped a lot in Symfony 1. It's out of scope but an optional panel could be nice. Ini flags are of great help when debugging on a hurry. :+1: for that! --------------------------------------------------------------------------- by Tobion at 2012-04-12T03:37:52Z @shiroyuki I don't understand your question. Everything of it should be displayed. But don't worry about phpinfo(), I'll work on that in a seperate PR. You can focus on the responsive design. ;) --------------------------------------------------------------------------- by vicb at 2012-04-12T06:54:35Z @shiroyuki I am not looking for anything specific. Just saying I have seen many times customer code using a publicly accessible file to return the info and it would help to get ride of this file. --------------------------------------------------------------------------- by sstok at 2012-04-12T07:59:18Z ``` should we support the toolbar for mobile device? ``` Good question, I don't think so because the screen-size is to small to show anything useful. Maybe a small icon to display the information as overlay, including the token so you can refer to that on a bigger screen?. --------------------------------------------------------------------------- by johnnypeck at 2012-04-13T06:45:43Z If your interested in a useful but not so intrusive way of providing the toolbar on mobile devices perhaps take a look at what the guys at Twitter have done with the topbar navigation converting to a semi-accordion style menu on mobile in Bootstrap. I can see the usefulness. Checkout the responsive.less which makes it easy enough to include/exclude depending on screen size. I found it quite useful in a recent project. Regarding adding a tab for phpinfo, sure it would be useful BUT if the reasoning is that some people leave a publicly available phpinfo script therefore just include it then I would not include it. There are many more useful requirements of the toolbar rather than to insulate intro to web issues. That's like saying don't include the toolbar because someone may build an application that makes the toolbar available publicly (which will happen). I've seen too many projects in my years having no clue of versioning tools that must have been built on the server, live, with filenames like indexv1.php, indexv2.php, indexTryAgain.php, db credentials in the clear, and just hoping to find a point where it works enough. And yes, I've found those scripts were publicly available and still around years after they were created; security holes and all! I'm preaching to the choir here. You'll never stop stupid. All we can do is educate by any means we have and share our knowledge with one another. Aside from that devils advocate reasoning, I would include the phpinfo tab, it does make sense in those random "did I/they compile that in" circumstances. ;-) Sorry for the rant. +1 for mobile sorta+1 for phpinfo +10 for better educating on how to include anything you need so phpinfo could be a "my first foray into adding a tool to my toolbar for Symfony" tutorial in the cookbook. Again, sorry for the long winded rant. Cheers everyone. Goodnight. --------------------------------------------------------------------------- by shiroyuki at 2012-04-13T23:33:21Z @stof I think we can remove the CSS.
The toolbar is very useful and containing lots of information. However, as there are too much information, it is very distracting and the toolbar area somehow ends up taking too much space and then becomes something like a panel.
The main purpose of this pull request is to hide any information and show only whenever the user wants to see, except the status code and response time.
This is based on the pull request #3833 with the feedbacks and for 2.1 (master).
The testing app is available at http://home.shiroyuki.com.