Display a better error design when the toolbar cannot be displayed #23274

yceruto · 2017-06-23T05:49:03Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes (failure unrelated)
Fixed tickets	#23266
License	MIT
Doc PR	n/a

Fixing the left position of the bar (tested in app without style) and escaping the literal newline (ES5) as some IDEs fails with previous syntax (and Github diff too). Btw, I have added the Symfony icon to make clear that this message comes from Symfony ;)

Before:

After:

stof · 2017-06-23T12:41:55Z

shouldn't it use the sfwdt{{ token }} element instead of inserting a whole new div too ?

stof · 2017-06-23T12:43:00Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_js.html.twig

+                        .sfErrorToolbar .sf-toolbar-icon { float: left; padding: 5px 0; margin-right: 10px; }\
+                    </style>\
+                    <div class="sfErrorToolbar">\
+                        <div class="sf-toolbar-icon"><svg width="26" height="28" xmlns="http://www.w3.org/2000/svg" version="1.1" x="0px" y="0px" viewBox="0 0 26 28" enable-background="new 0 0 26 28" xml:space="preserve"><path fill="#FFFFFF" d="M13 0C5.8 0 0 5.8 0 13c0 7.2 5.8 13 13 13c7.2 0 13-5.8 13-13C26 5.8 20.2 0 13 0z M20 7.5 c-0.6 0-1-0.3-1-0.9c0-0.2 0-0.4 0.2-0.6c0.1-0.3 0.2-0.3 0.2-0.4c0-0.3-0.5-0.4-0.7-0.4c-2 0.1-2.5 2.7-2.9 4.8l-0.2 1.1 c1.1 0.2 1.9 0 2.4-0.3c0.6-0.4-0.2-0.8-0.1-1.3C18 9.2 18.4 9 18.7 8.9c0.5 0 0.8 0.5 0.8 1c0 0.8-1.1 2-3.3 1.9 c-0.3 0-0.5 0-0.7-0.1L15 14.1c-0.4 1.7-0.9 4.1-2.6 6.2c-1.5 1.8-3.1 2.1-3.8 2.1c-1.3 0-2.1-0.6-2.2-1.6c0-0.9 0.8-1.4 1.3-1.4 c0.7 0 1.2 0.5 1.2 1.1c0 0.5-0.2 0.6-0.4 0.7c-0.1 0.1-0.3 0.2-0.3 0.4c0 0.1 0.1 0.3 0.4 0.3c0.5 0 0.9-0.3 1.2-0.5 c1.3-1 1.7-2.9 2.4-6.2l0.1-0.8c0.2-1.1 0.5-2.3 0.8-3.5c-0.9-0.7-1.4-1.5-2.6-1.8c-0.8-0.2-1.3 0-1.7 0.4C8.4 10 8.6 10.7 9 11.1 l0.7 0.7c0.8 0.9 1.3 1.7 1.1 2.7c-0.3 1.6-2.1 2.8-4.3 2.1c-1.9-0.6-2.2-1.9-2-2.7c0.2-0.6 0.7-0.8 1.2-0.6 c0.5 0.2 0.7 0.8 0.6 1.3c0 0.1 0 0.1-0.1 0.3C6 15 5.9 15.2 5.9 15.3c-0.1 0.4 0.4 0.7 0.8 0.8c0.8 0.3 1.7-0.2 1.9-0.9 c0.2-0.6-0.2-1.1-0.4-1.2l-0.8-0.9c-0.4-0.4-1.2-1.5-0.8-2.8c0.2-0.5 0.5-1 0.9-1.4c1-0.7 2-0.8 3-0.6c1.3 0.4 1.9 1.2 2.8 1.9 c0.5-1.3 1.1-2.6 2-3.8c0.9-1 2-1.7 3.3-1.8C20 4.8 21 5.4 21 6.3C21 6.7 20.8 7.5 20 7.5z"/></svg></div>\


could the SVG icon be maintained separately, like other icons we have ?

Well, I thought that but in 2.7 branch there isn't icons maintained separately yet, and in 2.8 there is already one, so I guess this could be refactored when this is merged in 2.8?

yceruto · 2017-06-23T13:55:44Z

shouldn't it use the sfwdt{{ token }} element instead of inserting a whole new div too ?

I think makes sense use the current elements to avoid empty div, respect the configured toolbar position and style mess, Updated!

yceruto · 2017-06-23T14:33:01Z

Fixed position when the toolbar is configured to the top.

Ready!

stof · 2017-06-23T14:38:33Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar.css.twig

@@ -440,3 +440,38 @@
        visibility: hidden;
    }
 }
+
+/***** Error Toolbar *****/
+.sfErrorToolbar {


can't you apply the sf-toolbarreset class on your error toolbar too ? This would avoid duplicating many things (your error toolbar is missing the resetting of box-sizing for instance, and a few other things).

This would also handle the positioning automatically.

btw, not having the sf-toolbar class means you also need to handle the case of the error toolbar separately in the print version.

yceruto · 2017-06-23T16:23:26Z

Well, after several adjustments the result remains the same (tested until version 3.4) :)

@stof thanks! any other suggestions?

yceruto · 2017-06-23T16:51:44Z

btw, not having the sf-toolbar class means you also need to handle the case of the error toolbar separately in the print version.

/***** Media query print: Do not print the Toolbar. *****/
@media print {
    .sf-toolbar {
        display: none;
        visibility: hidden;
    }
}

Btw, this style is present in 2.7 but was removed since 2.8 so the toolbar is displayed in the print version, maybe @javiereguiluz could validate those changes?

javiereguiluz · 2017-06-25T20:36:47Z

@yceruto I don't remember this change ... but thinking about this now, in my opinion is OK to print the debug toolbar ... if you want to print your web app, you probably display it in prod. Being able to print the debug toolbar can be interesting in some scenarios (think about reports, audits, etc. to show the performance, number of database queries, etc.)

yceruto · 2017-06-26T11:26:05Z

@javiereguiluz I agree with you.

We're fine then, this PR is ready.

ogizanagi · 2017-06-26T11:53:44Z

I don't agree about printing the debug toolbar. In most cases it does not make sense to me, and for the few case you'll need it, you can still create a new css rule or remove it using browser inspections.
If you're doing PDF generation from an HTML view for instance, it does not make sense to include the toolbar in dev mode.

I think it was simply forgotten when redesigning the profiler

yceruto · 2017-06-26T12:21:42Z

@ogizanagi I agree with you too, I've opened an issue to discuss about it #23303 as this issue starts since 2.8 (no related to this PR) ;)

fabpot · 2017-07-03T07:01:31Z

Thank you @yceruto.

…isplayed (yceruto) This PR was squashed before being merged into the 2.7 branch (closes #23274). Discussion ---------- Display a better error design when the toolbar cannot be displayed | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes (failure unrelated) | Fixed tickets | #23266 | License | MIT | Doc PR | n/a Fixing the left position of the bar (tested in app without style) and escaping the literal newline (ES5) as some IDEs fails with previous syntax (and Github diff too). Btw, I have added the Symfony icon to make clear that this message comes from Symfony ;) **Before:** ![current_toolbar_error](https://user-images.githubusercontent.com/2028198/27466735-cd5f0da8-57aa-11e7-8431-3025c41557e6.png) **After:** ![after_toolbar_27](https://user-images.githubusercontent.com/2028198/27467928-75e45d4e-57b4-11e7-9e1f-e252d9085596.png) Commits ------- ed3e403 Display a better error design when the toolbar cannot be displayed

carsonbot added Status: Needs Review Bug labels Jun 23, 2017

Display a better error design when the toolbar cannot be displayed

ee42fba

yceruto force-pushed the toolbar branch from 410aecc to ee42fba Compare June 23, 2017 11:22

stof reviewed Jun 23, 2017

View reviewed changes

Use current sfwdt div and pushing styles into toolbar.css.twig

86750a3

yceruto force-pushed the toolbar branch from f378760 to 86750a3 Compare June 23, 2017 14:12

stof reviewed Jun 23, 2017

View reviewed changes

Respect configured toolbar position

f733186

yceruto force-pushed the toolbar branch from bbe09fe to f733186 Compare June 23, 2017 16:05

Guaranteeing that the svg height is the same until 3.4 version

e40c409

yceruto mentioned this pull request Jun 26, 2017

[WebProfilerBundle] The debug toolbar is displayed in the printed version #23303

Closed

fabpot approved these changes Jul 3, 2017

View reviewed changes

fabpot closed this Jul 3, 2017

yceruto deleted the toolbar branch July 3, 2017 12:01

This was referenced Jul 3, 2017

Release v2.7.30 #23361

Merged

Release v2.8.23 #23367

Merged

Release v3.2.10 #23368

Merged

Release v3.3.3 #23369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display a better error design when the toolbar cannot be displayed #23274

Display a better error design when the toolbar cannot be displayed #23274

yceruto commented Jun 23, 2017 •

edited

Loading

stof commented Jun 23, 2017

stof Jun 23, 2017

yceruto Jun 23, 2017 •

edited

Loading

yceruto commented Jun 23, 2017 •

edited

Loading

yceruto commented Jun 23, 2017

stof Jun 23, 2017

stof Jun 23, 2017

yceruto Jun 23, 2017

yceruto commented Jun 23, 2017

yceruto commented Jun 23, 2017 •

edited

Loading

javiereguiluz commented Jun 25, 2017

yceruto commented Jun 26, 2017

ogizanagi commented Jun 26, 2017 •

edited

Loading

yceruto commented Jun 26, 2017 •

edited

Loading

fabpot commented Jul 3, 2017

Display a better error design when the toolbar cannot be displayed #23274

Display a better error design when the toolbar cannot be displayed #23274

Conversation

yceruto commented Jun 23, 2017 • edited Loading

stof commented Jun 23, 2017

stof Jun 23, 2017

Choose a reason for hiding this comment

yceruto Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

yceruto commented Jun 23, 2017 • edited Loading

yceruto commented Jun 23, 2017

stof Jun 23, 2017

Choose a reason for hiding this comment

stof Jun 23, 2017

Choose a reason for hiding this comment

yceruto Jun 23, 2017

Choose a reason for hiding this comment

yceruto commented Jun 23, 2017

yceruto commented Jun 23, 2017 • edited Loading

javiereguiluz commented Jun 25, 2017

yceruto commented Jun 26, 2017

ogizanagi commented Jun 26, 2017 • edited Loading

yceruto commented Jun 26, 2017 • edited Loading

fabpot commented Jul 3, 2017

yceruto commented Jun 23, 2017 •

edited

Loading

yceruto Jun 23, 2017 •

edited

Loading

yceruto commented Jun 23, 2017 •

edited

Loading

yceruto commented Jun 23, 2017 •

edited

Loading

ogizanagi commented Jun 26, 2017 •

edited

Loading

yceruto commented Jun 26, 2017 •

edited

Loading