[WebProfilerBundle] Load toolbar "forever", allow users to cancel #41257
Conversation
On projects where finishing the request takes some time, loading should not be stopped after a definitive amount of time. The loading bar will only be shown after the original amount of requests (5) were unsuccessful to flash as little as possible. Also the hold-off has been changed to 500 and a maximum of 2000.
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution 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 |
|
Hey! I think @Chi-teck has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
| if (count > 5) { | ||
| that.initToolbar(token); | ||
| } |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig
Show resolved
Hide resolved
|
@aleho I push few fix in your repo
@symfony/mergers PR RFR |
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/cancel.html.twig
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/cancel.html.twig
Outdated
Show resolved
Hide resolved
|
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 needed The 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
|
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.