Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier][WebProfilerBundle][FrameworkBundle] Add notifier section to profiler#36479
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
| {%setevents=collector.events %} | ||
| <spanclass="label {{events.messages|length?'':'disabled' }}"> | ||
| <spanclass="icon">{{include('@WebProfiler/Icon/mailer.svg') }}</span> |
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.
- exchange icon
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.
@javiereguiluz Can you work on an icon for the notifier profiler?
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.
Sorry for being late, but our designer just sent us the icon for the notifier. It uses the "bell icon" which is universal for notifications:
<svgxmlns="http://www.w3.org/2000/svg"height="24"width="24"viewBox="0 0 24 24"><pathfill="#AAA"d="M11.23,4.8c-3,.83-5.07,3.18-5.07,5.9H7.39c0-2.21,1.77-4,4.17-4.72ZM7.49,0A12.22,12.22,0,0,0,0,11.59H2.07A10.14,10.14,0,0,1,8.23,2Zm8.24,2a10.14,10.14,0,0,1,6.16,9.64H24A12.24,12.24,0,0,0,16.47,0ZM4.41,15.64V10.7a7.57,7.57,0,0,1,15.14,0v4.94l3.3,4.4H1.11Zm4.45,5.3A3.06,3.06,0,0,0,11.92,24H12a3.07,3.07,0,0,0,3.07-3.06Z"/></svg>
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.
@javiereguiluz as this PR is already merged, it might be better if you create a new PR with the new icon instead.
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@javiereguiluz I created a new PR:#38545
fabpot commentedAug 6, 2020
@jschaedl Will you have time to finish this PR? Any blockers? |
fabpot commentedAug 13, 2020
We might need to take into account the new |
jschaedl commentedAug 17, 2020
c87bf07 to77a07d9Compare77a07d9 to3af0d99Comparefabpot commentedSep 30, 2020
@jschaedl Do you think you will have something that we can merge before the end of the week (for 5.2)? Even if it's not finished? |
jschaedl commentedOct 1, 2020
Hi@fabpot, yes I think so. I was planning to work on it tmrw again and will let you know how far I've come. |
7ecc8f9 to45e23a7Comparejschaedl commentedOct 2, 2020
@fabpot Ready for review. :-) |
| publicfunctionsend(MessageInterface$message):SentMessage | ||
| { | ||
| if (null ===$message->getTransport()) { | ||
| $message->transport((string)$this); |
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 needed to make sure that every transport has a name to show this name in the profiler panel.
| publicfunctiongetTransport(): ?string; | ||
| publicfunctiontransport(string$transport):self; |
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.
Not a big fan, but I don't have any better idea for now.
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.
Maybe we could decorate the message with aDebugMessage that exposes this information?
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 like this idea and gave it a try:#38474 WDYT?
fabpot commentedOct 4, 2020
Thank you@jschaedl. |
45e23a7 tof39e74bCompare…tter in MessageInterface (jschaedl)This PR was merged into the 5.x branch.Discussion----------[Notifier] Introduce NullMessage and remove transport setter in MessageInterface| Q | A| ------------- | ---| Branch? | 5.x| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets | - <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | - <!-- required for new features -->Follow-up PR of#36479Commits-------5701e89 Introduce NullMessage and remove transport setter in MessageInterface
…l icon (jschaedl)This PR was squashed before being merged into the 5.x branch.Discussion----------[Notifier][WebProfilerBundle] Add notifier profiler panel icon| Q | A| ------------- | ---| Branch? | 5.x| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets || License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features --><!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/releases): - Always add tests and ensure they pass. - Never break backward compatibility (seehttps://symfony.com/bc). - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too.) - Features and deprecations must be submitted against branch 5.x.-->relates to#36479Commits-------303096e [Notifier][WebProfilerBundle] Add notifier profiler panel icon
Uh oh!
There was an error while loading.Please reload this page.
This is the first iteration of adding a profiler panel for the new Notifier component:
WebProfiler Toolbar:
WebProfiler Notifier Panel:
An example project can to test the new profiler panel can be found here:https://github.com/jschaedl/notifier-profiler-integration