Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebProfilerBundle] Load toolbar "forever", allow users to cancel#41257
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
On projects where finishing the request takes some time, loadingshould not be stopped after a definitive amount of time.The loading bar will only be shown after the original amount ofrequests (5) were unsuccessful to flash as little as possible.Also the hold-off has been changed to 500 and a maximum of 2000.
carsonbot commentedMay 17, 2021
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
carsonbot commentedMay 18, 2021
Hey! I think@Chi-teck has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
jderusse left a comment
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.
Works for me except for a little bug when the user click oncancel the toolbar disappear then re-appear
| if (count>5) { | ||
| that.initToolbar(token); | ||
| } |
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.
It means we are starting to show the toolbar after the 5th attempt.
I wonder if we shouldn't show the toolbar immediately (or at least after one or 2 seconds)
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twigShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
jderusse commentedNov 2, 2021
@aleho I push few fix in your repo
@symfony/mergers PR RFR |
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/cancel.html.twig OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/cancel.html.twig OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedNov 3, 2021
Thank you@aleho. |
…'s needed by toolbar.html.twig (weaverryan)This PR was merged into the 5.4 branch.Discussion----------[5.4][WebProfiler] Fixing missing full_stack variable that's needed by toolbar.html.twig| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | None| License | MIT| Doc PR | Not neededThe combination of#43526 and then#41257 created an undefined variable. By adding the variable here, it will flow from `toolbar_js.html.twig` into `toolbar.html.twig`.Tested locally after reproducing the issue. Thanks to symfony/ux test suite for catching this :)Commits-------0e8b864 Fixing missing full_stack variable that's needed by toolbar.html.twig
aleho commentedNov 4, 2021
Thanks guys! I was down with a cold only to "wake up" to this being improved and merged! |
On projects where finishing the request takes some time, loading should not be stopped after a definitive amount of time.
The latest changes introducing an increasing hold-off timer make failures to load the toolbar less likely, although not impossible. They also increase the loading times too much in a too short period of time in my opinion and they don't account for the most common cases where only a short delay is needed.
These changes make the toolbar appear almost instantly in the projects I tested, even if the first request failed.
The toolbar with loading counter will only be shown after the original amount of requests (5) were unsuccessful to flash as little as possible. It allows to cancel the loop, if needed.