Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Updated the Messenger profiler panel#26674
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
linaori commentedMar 26, 2018
"Message Bus" for me 👍, the other looks like a chat icon. |
ro0NL commentedMar 26, 2018 • 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.
"Messaging" ? i also suggestedhttps://www.materialui.co/icon/message to@sroze to me multiple baloons and thingies only clutter the icon. |
ro0NL commentedMar 26, 2018 • 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.
to proposed message bus icon (small toolbar version) to me looks like a routing thing, ie.http://c8.alamy.com/comp/HHAYN4/travel-tourism-distance-signposts-tour-route-direction-pole-data-HHAYN4.jpg |
sroze commentedMar 26, 2018
I quite like the "Message Bus" option, that shows the notion of routing (to/from adapters) behind the message bus(es). |
ro0NL commentedMar 26, 2018 • 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.
Large version are nice, it's just the small version that might defeat its purpose, i.e. are hard to identify. IMHO icons like mail and security, thus more solid, are better candidates here. |
fabpot commentedMar 26, 2018
"Message Bus" looks good to me. |
stof commentedMar 26, 2018
The icon still looks weird at small size in the screenshot. Would it be possible to simplify the icon to optimize it for display at small size (the toolbar will always display it at 24px) |
| {% block toolbar %} | ||
| {% set color_code = 'normal' %} | ||
| {% set message_count = 0 %} |
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.
wrong count here
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.
This is going to be fixed in#26650
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.
Removed
| <span class="sf-toolbar-value">{{ message_count }}</span> | ||
| {% endset %} | ||
| {{ include('@WebProfiler/Profiler/toolbar_item.html.twig', { link: 'messages', status: color_code }) }} |
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.
you could even avoid passing the status explicitly, as you never change it to anything else thannormal (which is the default status)
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.
Removed too!
| <span class="icon">{{ include('@WebProfiler/Icon/messenger.svg') }}</span> | ||
| <strong>Messages</strong> | ||
| {% if collector.messages|length > 0 %} |
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.
IIRC, we removed all counts from the profiler menu in the redesign done in 2.8. Why having it here ?
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.
True ... but I've moved this to{% block toolbar %} because the Messenger must only be displayed if there are messages, like we do with the rest of panels.
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.
Well, you haven't removed the count itself in the profiler menu
andreybolonin commentedMar 26, 2018
+1 "Message Bus" |
fabpot commentedMar 27, 2018
Thank you@javiereguiluz. |
This PR was squashed before being merged into the 4.1-dev branch (closes#26674).Discussion----------Updated the Messenger profiler panel| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -The profiler icon of the new Messenger component was still pending. After talking with@sroze, we settled on two possible ideas for the icon: 1) focus on "messages"; 2) focus on "bus" / "message bus".These are the icons for the two proposals:| "Messages" | "Message Bus"| --- | ---|  | This is how they look inside the profiler:"Messages""Message Bus"-----My opinion:* "Messages" look like "forum messages" or "chat messages". It looks wrong to me.* Inside the profiler, "Messages" looks similar to "Translation" and we already have another "messages" icon for the "Mailer".* For these reasons, I'd use "Message Bus".Now, let's hear the community opinion. Thanks!-----If you want to test then on your computer:"Messages"```svg<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="#aaa" d="M11.32 9.06h-8a3.07 3.07 0 0 0-3.35 3v5a3.09 3.09 0 0 0 3.35 3h2.75l4 3.69c.42.35.89.21.89-.34v-3c2 0 4-1.5 4-3.35v-5c.04-1.85-1.79-3-3.64-3zM11 16.5H4a.5.5 0 1 1 0-1h7a.5.5 0 0 1 0 1zm0-2.63H4a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.5h7a.5.5 0 0 1 .5.5.5.5 0 0 1-.5.5zM21.29.06h-6.46A2.83 2.83 0 0 0 12 2.89v5.33c3.31 0 4 2.28 4 4.72 0 .44.46.56.8.28l3.31-3.16h1.21A2.71 2.71 0 0 0 24 7.35V2.77A2.71 2.71 0 0 0 21.29.06zm-.81 6.66h-5a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.5h5a.5.5 0 0 1 .5.5.5.5 0 0 1-.5.5zm0-2.63h-5a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.5h5a.5.5 0 0 1 .5.5.5.5 0 0 1-.5.5z"/></svg>```"Message Bus"```svg<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="#aaa" d="M16 9a2 2 0 0 0 2 2h4a2 2 0 0 0 2-2V7a2 2 0 0 0-2-2h-4a2 2 0 0 0-2 2h-3V4a1 1 0 0 0-1-1H8a2 2 0 0 0-2-2H2a2 2 0 0 0-2 2v2a2 2 0 0 0 2 2h4a2 2 0 0 0 2-2h3v6H8a2 2 0 0 0-2-2H2a2 2 0 0 0-2 2v2a2 2 0 0 0 2 2h4a2 2 0 0 0 2-2h3v9a1 1 0 0 0 2 0v-5h3a2 2 0 0 0 2 2h4a2 2 0 0 0 2-2v-2a2 2 0 0 0-2-2h-4a2 2 0 0 0-2 2h-3V9zm2.52-2.5h3a.5.5 0 0 1 0 1h-3a.5.5 0 0 1 0-1zm0 1.63h3a.5.5 0 0 1 .5.5.5.5 0 0 1-.5.5h-3a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.52zm-13-2.82h-3a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.5h3a.5.5 0 0 1 .5.5.5.5 0 0 1-.54.48zm0-1.62h-3a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.5h3a.5.5 0 0 1 .5.5.5.5 0 0 1-.54.48zm0 9.62h-3a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.5h3a.5.5 0 0 1 .5.5.5.5 0 0 1-.54.48zm0-1.62h-3a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.5h3a.5.5 0 0 1 .5.5.5.5 0 0 1-.54.48zm13 2.81h3a.5.5 0 0 1 0 1h-3a.5.5 0 0 1 0-1zm0 1.63h3a.5.5 0 0 1 .5.5.5.5 0 0 1-.5.5h-3a.5.5 0 0 1-.5-.5.5.5 0 0 1 .5-.52z"/></svg>```Commits-------92a041c Updated the Messenger profiler panel
This PR was merged into the 4.1-dev branch.Discussion----------[Messenger] Fix messenger profiler name| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#26674| License | MIT| Doc PR | øIn#26674, the profiler name was not renamed properly, which means it wasn't displayed anymore.Commits-------bd234ff Fix messenger profiler name
The profiler icon of the new Messenger component was still pending. After talking with@sroze, we settled on two possible ideas for the icon: 1) focus on "messages"; 2) focus on "bus" / "message bus".
These are the icons for the two proposals:
This is how they look inside the profiler:
"Messages"
"Message Bus"
My opinion:
Now, let's hear the community opinion. Thanks!
If you want to test then on your computer:
"Messages"
"Message Bus"