search

turbolinks / turbolinks-classic

Classic version of Turbolinks. Now deprecated in favor of Turbolinks 5.
MIT License
3.54k stars 431 forks source link

Delay before showing progress bar #604

Closed monkbroc closed 9 years ago

monkbroc commented 9 years ago

The progress bar makes loading feel slower for fast requests as @javan noted in #581.

This PR makes _updateStyle a no-op until a set delay has passed (default 400ms).

There is another implementation in #593 but it only delays the initial advanceTo call, not subsequent ones.

rails-bot commented 9 years ago

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @Thibaut (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

monkbroc commented 9 years ago

r? @javan

javan commented 9 years ago

I don't think the delay belongs in ProgressBar. Instead, fetch should use setTimeout to delay displaying it.

monkbroc commented 9 years ago

OK. If you think the delay should live somewhere else I can just close this PR.

My reasoning for this implementation was to avoid timing issues by keeping the method call sequence the same (start --> advanceTo --> done). If start is delayed using setTimeout what happens if done is called before start? The start timer will need to be cancelled.

The other reason was to keep all the logic related to ProgressBar encapsulated. Other code using the class don't need to care if it's delayed or not. Another thought I had was adding an optional parameter to start specifying the delay so users could get an immediate or delayed progress bar in their own Javascript.

javan commented 9 years ago

Hey @monkbroc, I took a shot at implementing this using your idea to pass an option to start: https://github.com/rails/turbolinks/pull/608. Let me know what you think.