Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix dump panel hidden when closing a dump#24665
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
nicolas-grekas commentedOct 23, 2017
I didn't try the behavior on a page where the dump is displayed inline. Does this change anything in this situation? (eg in the dump() panel, or when using |
julienfalque commentedOct 23, 2017
Didn't try either but it's worth checking before merging. I'll have a look. |
b1d290f tob16ffd2Comparejulienfalque commentedOct 23, 2017
I updated the PR: previous version was not working with inline dumps (the I also noticed that all dumps have a |
ogizanagi commentedOct 24, 2017
@julienfalque : I still get a |
b16ffd2 toece2e1fComparejulienfalque commentedOct 24, 2017
Woops. Should be fixed now. |
| arrow = '▶'; | ||
| newClass = 'sf-dump-compact'; | ||
| if ('undefined' !== typeof window.Sfdump.debugBarPanel) { |
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 means var-dumper knows something about the Symfony panel.
Can't we decouple things and alter the logic in the panel's twig file 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.
Done. I used a custom event to decouple toggle detection and panel min-height handling.
55a7515 to4b58a83Compare| return false; | ||
| } | ||
| s.dispatchEvent(new CustomEvent('sfbeforedumptoggle', { |
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.
breaks on IE11?
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.
We don't care about IE for Symfony development tools (we do care about Microsoft Edge though).
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.
we do care about IE11 for things running in the toolbar, because this one is injected in the page itself, and so anything breaking the JS could break the page itself.
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.
note that today, IE11 supports the WDT properly (we just had an issue with the SVG rendering in the past doing wonky stuff when the size was not defined in the SVG, and we fixed it)
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 disagree. Symfony debug toolbars are related to the backend. If you have a Symfony-related problem in your app, you should be able to use any browser. So there's no need to support legacy browsers. The only exception are Ajax requests ... but that's two different things: you can use IE to debug frontend + JS errors related to Ajax and any other browser to debug the Symfony responses to those Ajax requests.
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.
At least failing gracefully should be required IMHO.
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 but if the frontend is brokenbecause of the JS injected by the toolbar, you cannot debug the frontend because the toolbar causes extra errors.
It is OK if some feature does not work. It is not OK for the JS to break.
nicolas-grekas commentedOct 28, 2017
Status: needs work (should degrade gracefully on non-supported OSes) |
4b58a83 to70ba05aComparejulienfalque commentedOct 30, 2017
Rewritten using |
| } | ||
| var event = doc.createEvent('Event'); | ||
| event.initEvent( |
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 call should be on one line.
to further enhance compat, what about wrapping the new logic in a check like:if (doc.createEvent) {...}?
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.
Updated.
70ba05a to2b9b6e0Compare| event.initEvent('sf-dump-expanded' === newClass ? 'sfbeforedumpexpand' : 'sfbeforedumpcollapse', true, false); | ||
| } | ||
| s.dispatchEvent(event); |
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.
should be inside the "if"?
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.
Oops, indeed... Also added to theif condition.
2b9b6e0 to2e0b263Comparefabpot commentedNov 5, 2017
Thank you@julienfalque. |
This PR was merged into the 2.8 branch.Discussion----------Fix dump panel hidden when closing a dump| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes-ish| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -In the dump panel of the debug bar, when closing a dump the panel sometimes get hidden:This is because when the size of the panel is reduced, if the mouse is not over it anymore, the `:hover` pseudo-class does not apply anymore.I "fixed" it by setting a min-height on the panel when closing a dump. The min-height is removed when leaving the panel _on purpose_:For now I only tested it on Firefox 56 on Arch Linux.Commits-------2e0b263 Fix dump panel hidden when closing a dump
Uh oh!
There was an error while loading.Please reload this page.
In the dump panel of the debug bar, when closing a dump the panel sometimes get hidden:
This is because when the size of the panel is reduced, if the mouse is not over it anymore, the
:hoverpseudo-class does not apply anymore.I "fixed" it by setting a min-height on the panel when closing a dump. The min-height is removed when leaving the panelon purpose:
For now I only tested it on Firefox 56 on Arch Linux.