Update the toolbar and panel for the new profiler design #109

javiereguiluz · 2015-07-30T13:18:48Z

This PR adds support for the new Symfony Toolbar (see symfony/symfony#15160) and it uses the same trick validated by @stof for the DoctrineBundle (see doctrine/DoctrineBundle#445).

stof · 2015-07-30T15:56:28Z

why are you reverting an empty commit ? It looks to me you are missing the point of git revert :)

sstok · 2015-07-30T18:05:03Z

@javiereguiluz git commit --amend also creates a "new" commit which you can force push ;)

stof · 2015-07-30T18:06:26Z

anyway, I can just squash the empty commits with gh before merging the changes, but this will wait until the new toolbar design is accepted in symfony itself

javiereguiluz · 2015-08-24T07:24:41Z

This pull request is now ready for the final review. It contains the redesign of both the toolbar and the profiler.

wouterj · 2015-08-25T20:03:50Z

It's called "E-mails" in the menu and "Swiftmailer" in the page title. That's inconsistent imo and I propose to change the title to "E-mails".

wouterj · 2015-08-25T20:05:05Z

Also, all other panels show a dashed border + a nice message when no info is shown. In the case of emails, there is a table with only the table head visible (as there is no table body). Imo, there should be the same nice dash-bordered box as in other panels.

javiereguiluz · 2015-08-26T06:21:25Z

@wouterj about the dashed border, when no emails are sent, the following code is executed:

        {% if not collector.mailers %}
            <div class="empty">
                <p>No e-mail messages were sent.</p>
            </div>
        {% endif %}

At least locally is working for me:

javiereguiluz · 2015-08-26T06:47:21Z

It's called "E-mails" in the menu and "Swiftmailer" in the page title. That's inconsistent imo and I propose to change the title to "E-mails".

You are right about this inconsistency. Sadly, most panels use a name different than their menu option. I've fixed it in symfony/symfony@43f5639 but maybe the best solution would be to not show the panel name in the page title.

wouterj · 2015-08-26T07:16:42Z

@javiereguiluz as mentioned in the other PR, I forgot I also had to bring a new version of Swiftmailer. So I actually reviewed the old design :)

stof · 2015-08-26T07:19:12Z

You are right about this inconsistency. Sadly, most panels use a name different than their menu option. I've fixed it in symfony/symfony@43f5639 but maybe the best solution would be to not show the panel name in the page title.

The solution might be to let the panel provide the text displayed in the title

stof · 2015-08-26T07:21:09Z

Resources/views/Collector/swiftmailer.html.twig

-            {% for name in collector.mailers %}
+    {% set profiler_markup_version = profiler_markup_version|default(1) %}
+
+    {% if profiler_markup_version == 1 %}


Can we avoid duplicating the whole panel between the old and the new design of the profiler, finding a markup looking fine in both designs ? I would be much easier to maintain

I've tried it ... but there are so many little differences between both designs that I cannot deduplicate them.

well, this may mean we should make some changes in the new design PR in Symfony to make it easier to share markup. Maintaining support for both designs at the same time will be a must have for all bundles providing a webprofiler panel for at least 3 years if they want to keep support for the 2.7 LTS.

As I was testing the old design yesterday with the new profiler, it actually didn't look that bad (besides the menu item). Is it really required to have a new design, or is this just a redesign to update all core packages with a new design?

The problem here is that we're doing two different things at the same time: 1) update the design of the panel; 2) improve the panel contents.

If the old panel's contents are OK, you only have to do minimal changes ... but that's not the case for this panel.

So that means this PR doesn't actually show the impact of the new profiler design for packages. If they just use the same design as with the old design, they only have to do minimal changes.

I think that's what @stof wants to see: A real example of what a bundle has to update with the redesign. Maybe you can show this by updating another bundle with only the required minimal changes (like SonataBlockBundle)?

In EasyAdmin I recently added a profiler compatible with both the old and the new design: https://github.com/javiereguiluz/EasyAdminBundle/pull/413/files

The changes are:

I use a single SVG icon for the menu item, but I change its color depending on the profiler version. This is an optional change to make it look "perfect" in both versions.

In the old profiler I use a table and in the new one I use the .metrics element. Again this is optional and I could use the exact same code for the new profiler. I did this because the metrics look cool and the table is boring.

The .tabs element of the new profiler degrades gracefully, so you can see that I reuse the exact asme tabs in the old and the new profiler.

In summary, I could change nothing, but I decided to change one icon color and one table to make it look better in the new profiler.

This PR does 2 things at the same time, but it improves the panel content only for people using the new design. This is where the issue is IMO. Content improvements should be applied to everyone (the content of this panel was not improved since a very long time, and the initial design of the panel was not thought to support multiple mailers, which is probably why it is not that good).

The EasyAdminBundle example is a very good example of what I'm aiming for: having only minor differences between versions, just to make it look better in places where the new version has a better way to display it which would not degrade gracefully in the old version.

javiereguiluz · 2015-08-26T11:19:07Z

@stof I think that in b3dd574 I finally did what you were looking for. We now use the exact same code for both the old and the new profiler ... except for some metrics/tables. The trick is to add some lines of CSS for the old profiler to make it look better.

This is how it looks the latest code of this PR inside an old Symfony application:

Usual scenario: 1 mailer 1 email

1 mailer 2 emails

2 mailers but only one sends emails

Most complex scenario: several mailers and several emails

stof · 2015-09-22T08:33:56Z

@javiereguiluz can you post a screenshot for a realistic use case for HTML parts of emails, i.e. a big HTML blob rather than a single line ? I was never able to write a real HTML email with such a small markup 😄

stof · 2015-09-22T08:34:50Z

Resources/views/Collector/swiftmailer.html.twig

+                <img width="23" height="28" alt="Swiftmailer" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABcAAAAcCAYAAACK7SRjAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAyJpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAAAADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4OnhtcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuMC1jMDYwIDYxLjEzNDc3NywgMjAxMC8wMi8xMi0xNzozMjowMCAgICAgICAgIj4gPHJkZjpSREYgeG1sbnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjpEZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC8xLjAvIiB4bWxuczp4bXBNTT0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wL21tLyIgeG1sbnM6c3RSZWY9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9zVHlwZS9SZXNvdXJjZVJlZiMiIHhtcDpDcmVhdG9yVG9vbD0iQWRvYmUgUGhvdG9zaG9wIENTNSBNYWNpbnRvc2giIHhtcE1NOkluc3RhbmNlSUQ9InhtcC5paWQ6N0NEOTU1MjM0OThFMTFFMDg3NzJBNTE2ODgwQzMxMzQiIHhtcE1NOkRvY3VtZW50SUQ9InhtcC5kaWQ6N0NEOTU1MjQ0OThFMTFFMDg3NzJBNTE2ODgwQzMxMzQiPiA8eG1wTU06RGVyaXZlZEZyb20gc3RSZWY6aW5zdGFuY2VJRD0ieG1wLmlpZDoxMEQ5OTQ5QzQ5OEMxMUUwODc3MkE1MTY4ODBDMzEzNCIgc3RSZWY6ZG9jdW1lbnRJRD0ieG1wLmRpZDo3Q0Q5NTUyMjQ5OEUxMUUwODc3MkE1MTY4ODBDMzEzNCIvPiA8L3JkZjpEZXNjcmlwdGlvbj4gPC9yZGY6UkRGPiA8L3g6eG1wbWV0YT4gPD94cGFja2V0IGVuZD0iciI/PpkRnSAAAAJ0SURBVHjaYvz//z8DrQATAw3BqOFYAaO9vT1FBhw4cGCAXA5MipxBQUHT3r17l0AVAxkZ/wkLC89as2ZNIcjlYkALXKnlWqBZTH/+/PEDmQsynLW/v3+NoaHhN2oYDjJn8uTJK4BMNpDhPwsLCwOKiop2+fn5vafEYC8vrw8gc/Lz8wOB3B8gw/nev38vn5WV5eTg4LA/Ly/vESsrK2npmYmJITU19SnQ8L0gc4DxpwgyF2S4EEjB58+f+crLy31YWFjOt7S0XBYUFPxHjMEcHBz/6+rqboqJiZ0qKSnxBpkDlRICGc4MU/j792+2CRMm+L18+fLSxIkTDykoKPzBZ7CoqOi/np6eE8rKylvb29v9fvz4wYEkzYKRzjk5OX/LyMjcnDRpEkjjdisrK6wRraOj8wvokAMLFy788ejRoxcaGhrPCWai4ODgB8DUE3/mzBknYMToASNoMzAfvEVW4+Tk9LmhoWFbTU2NwunTpx2BjiiMjo6+hm4WCzJHUlLyz+vXrxkfP36sDOI/ffpUPikpibe7u3sLsJjQW7VqlSrQxe+Avjmanp7u9PbtWzGQOmCCkARmmu/m5uYfT548yY/V5UpKSl+2b9+uiiz26dMnIWBSDTp27NgdYGrYCIzwE7m5uR4wg2Hg/PnzSsDI/QlKOcjZ3wGUBLm5uf+DwLdv38gub4AG/xcSEvr35s0bZmCB5sgCE/z69SsjyDJKMtG/f/8YQQYD8wkoGf8H51AbG5sH1CpbQBnQ09PzBiiHgoIFFHlBQGwLxLxUMP8dqJgH4k3gIhfIkAKVYkDMTmmhCHIxEL8A4peMo02L4WU4QIABANxZAoDIQDv3AAAAAElFTkSuQmCC" />
+                <span class="sf-toolbar-status">{{ collector.messageCount }}</span>
+            {% else %}
+                {{ include('@Swiftmailer/Collector/icon.svg') }}


the icon does not exist. You forgot to include it in the PR.

javiereguiluz · 2015-09-23T12:23:02Z

can you post a screenshot for a realistic use case for HTML parts of emails

Here it is a real example with a real HTML newsletter:

As you can see, the result is abominable. I propose to improve this as follows (in a separate pull request): I'd like to apply a max-height property on the email contents (as StackOverflow does for code blocks).

Another improvement (again in a separate PR) I'd like to do is to implement this great idea proposed by @garak to allow render the real HTML contents instead of just displaying the raw HTML tags.

javiereguiluz · 2015-09-23T13:14:15Z

Status of this PR: I've implemented all the changes proposed by @stof, there is a new proposal pending approval (javiereguiluz@7b5178d) and there are two other ideas that will be implemented in a separate PR.

stof · 2015-09-23T14:08:40Z

Can you post a screenshot of your new proposal ? I don't have a project using this bundle to try it locally (and even less a 2.8 project)

stof · 2015-09-23T14:10:22Z

Resources/views/Collector/swiftmailer.html.twig

+                        <span>{{ collector.isSpool(name) ? 'yes' : 'no' }}</span>
+                    </div>
+
+                    {% if not loop.first %}


are you sure about it ? Shouldn't it be either skipped for the last element instead, or displayed before the content for all except the first element ?

javiereguiluz · 2015-09-23T14:29:51Z

@stof the screenshots for the small redesign can be seen here: #109 (comment)

stof · 2015-09-23T15:27:15Z

Except it is totally unreadable (the width of the image is 149px), so I cannot see how the top of the page looks like

javiereguiluz · 2015-09-23T15:33:18Z

@stof we are not understanding each other :)

The unreadable image of this comment shows the current design with a real email. It just shows how bad long emails are displayed. This problem will be solved later in another PR.

The small redesign that I propose for this PR is explained in the three screenshots included in this other comment.

stof · 2015-09-28T14:36:05Z

@javiereguiluz can you rebase your branch to fix conflicts ? the merge of #112 is causing one

stof · 2015-09-28T14:36:17Z

the small redesign looks good to me btw

javiereguiluz · 2015-09-28T14:36:19Z

As said above, the two pending issues will be solved in different PRs. I'm going to rebase this to be able to merge it ASAP. Thanks.

javiereguiluz · 2015-09-28T14:39:58Z

Rebased.

fabpot · 2015-09-28T14:45:56Z

Thank you @javiereguiluz.

stof reviewed Aug 26, 2015
View reviewed changes

stof changed the title ~~Added BC support for the new Symfony Toolbar~~ Update the toolbar and panel for the new profiler design Sep 22, 2015

stof reviewed Sep 22, 2015
View reviewed changes

stof reviewed Sep 23, 2015
View reviewed changes

javiereguiluz added 17 commits September 28, 2015 16:39

Added BC support for the new Symfony Toolbar

59aca85

Empty commit to trigger webhook

9a1986b

Revert the previous empty commit

73c87be

Finished the redesign of the profiler pane

8659e93

Fixed an alignment issue

588b0e4

Remove a blank line

9e92592

Deduplicated some code

b487e6f

Improved design for the old version of the profiler

570cf3f

Set the page title explicitly and tweaked some styles

a81ee17

Profiler panels can no longer set their title

e6e410b

Added the new icon

89dc02d

Show th <hr> element only for non-first element

fad5d7c

Removed an unnecessary ternary operator condition

af8d55b

Minor Twig optimization

67e7906

Improved the way we escape conents before displaying them

58e19b1

Minor Twig optimization

830c3dd

Redesigned the section which summarizes the send messages

152dad0

javiereguiluz force-pushed the new_toolbar_support branch from 7b5178d to 152dad0 Compare September 28, 2015 14:39

javiereguiluz mentioned this pull request Sep 28, 2015

Improve the way long emails are displayed in the profiler #113

Closed

fabpot closed this in c7c15ca Sep 28, 2015

hason mentioned this pull request Oct 1, 2015

[feature] Render HTML email in Profiler #86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the toolbar and panel for the new profiler design #109

Update the toolbar and panel for the new profiler design #109

javiereguiluz commented Jul 30, 2015

stof commented Jul 30, 2015

sstok commented Jul 30, 2015

stof commented Jul 30, 2015

javiereguiluz commented Aug 24, 2015

wouterj commented Aug 25, 2015

wouterj commented Aug 25, 2015

javiereguiluz commented Aug 26, 2015

javiereguiluz commented Aug 26, 2015

wouterj commented Aug 26, 2015

stof commented Aug 26, 2015

stof Aug 26, 2015

javiereguiluz Aug 26, 2015

stof Aug 26, 2015

wouterj Aug 26, 2015

javiereguiluz Aug 26, 2015

wouterj Aug 26, 2015

javiereguiluz Aug 26, 2015

stof Aug 26, 2015

javiereguiluz commented Aug 26, 2015

stof commented Sep 22, 2015

stof Sep 22, 2015

javiereguiluz commented Sep 23, 2015

javiereguiluz commented Sep 23, 2015

stof commented Sep 23, 2015

stof Sep 23, 2015

javiereguiluz commented Sep 23, 2015

stof commented Sep 23, 2015

javiereguiluz commented Sep 23, 2015

stof commented Sep 28, 2015

stof commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

fabpot commented Sep 28, 2015

Update the toolbar and panel for the new profiler design #109

Update the toolbar and panel for the new profiler design #109

Conversation

javiereguiluz commented Jul 30, 2015

stof commented Jul 30, 2015

sstok commented Jul 30, 2015

stof commented Jul 30, 2015

javiereguiluz commented Aug 24, 2015

wouterj commented Aug 25, 2015

wouterj commented Aug 25, 2015

javiereguiluz commented Aug 26, 2015

javiereguiluz commented Aug 26, 2015

wouterj commented Aug 26, 2015

stof commented Aug 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Aug 26, 2015

stof commented Sep 22, 2015

Choose a reason for hiding this comment

javiereguiluz commented Sep 23, 2015

javiereguiluz commented Sep 23, 2015

stof commented Sep 23, 2015

Choose a reason for hiding this comment

javiereguiluz commented Sep 23, 2015

stof commented Sep 23, 2015

javiereguiluz commented Sep 23, 2015

stof commented Sep 28, 2015

stof commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

fabpot commented Sep 28, 2015