Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebProfilerBundle] Improve cache panel#22129
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 left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 trying to improve this page was on my TODO list for 3.3 too.@ro0NL thanks for taking care of this.
I agree that this page has too much information. Looking at your screenshot, I'd like to suggest these additional changes:
Use the
<span>ms</span>code to display the "ms" value, as we do in other profiler panels such as "Performance" (and maybe apply it to the "%" too in "hit/misses" ??)Rearrange the row of information as follows:
- Total calls
- Total time
<div></div>- Total reads
- Total writes
- Total deletes
<div></div>- Total Hits
- Total Misses
- Hits/reads
ro0NL commentedMar 23, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Improved the metric handling, moving things to the view layer. Totally worth it :) (screenshot updated). |
| <th>Results</th> | ||
| <td> | ||
| {%ifcall.result!=false %} | ||
| {{ profiler_dump(call.result, maxDepth=1) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Nyholm not sure about a) this check and b) loose comparison. As you can see in the screenshotsnull was hidden as wel. Doesfalse need to be excluded, strictly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for this PR.
I'm not sure about your questions. Call.result could be false if there was an error (if I remember correctly). Ithink that is the reason for this check.
Why it not is a strict comparison... that is probably a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll check it out tonight. I'd prefer something like displayingn/a in that case, opposed tojust an empty cell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Nyholm figured it out; this is (AFAIK) the direct result value from the method call passed by the traceableadapter.
Now givenhttps://github.com/php-fig/cache/blob/master/src/CacheItemInterface.php#L42 i dont think we should makyany assumptions here.
Besides, this hidesfalse as a result from ahasItem call ;-)
stof left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
ro0NL commentedMar 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Did some overall improvements to the table structure.. i kinda like it. The whole thing looks really calm now and we save a lot on spacing overall. Screenshots updated. Should be reviewed with |
fabpot commentedMar 24, 2017
Thank you@ro0NL. |
This PR was squashed before being merged into the 3.3-dev branch (closes#22129).Discussion----------[WebProfilerBundle] Improve cache panel| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->An attempt to improve the page a bit. Personally i think all elements on a single page is too much info.BeforeAfterCommits-------3592d0d [WebProfilerBundle] Improve cache panel
Uh oh!
There was an error while loading.Please reload this page.
An attempt to improve the page a bit. Personally i think all elements on a single page is too much info.
Before
After