hotwired / turbo

The speed of a single-page web application without having to write any JavaScript
https://turbo.hotwired.dev
MIT License
6.72k stars 428 forks source link

Progress bar is not shown when using Turbo Frames (even with turbo-action="advance") #540

Open nickjj opened 2 years ago

nickjj commented 2 years ago

Hi,

The documentation specifically mentions that the progress bar is for Turbo Drive but if you use Turbo Frames with URL history support such as when using <a href="/profile" data-turbo-frame="main" data-turbo-action="advance"> then it's somewhat expected that the progress bar would be shown here because this page transition may happen within 50ms or 5000ms due to external network events that's out of our control.

You can test that it's not being shown by adding a sleep 1 between Turbo Frame page transitions and not seeing the progress bar where as if you remove the frame so that Turbo Drive picks it up then the progress bar is shown. This is with v7.1.0.

Can we make a case here to show the progress bar for all Turbo Frame events regardless of it being done with turbo-action="advance"? I'm guessing this was very thoroughly investigated at Hey / Basecamp but that discussion isn't in the issue tracker here. Am I missing something important that would make showing the progress bar for all Turbo Frame events a bad idea?

dhh commented 2 years ago

We looked at invoking the progress bar for everything, but that didn't seem right. You can have a lot of frames triggering for lazy loading. It would be a very busy progress bar.

What I'd rather do is have an option to explicitly opt-in to using the progress bar when it's not a full page replace. Could be as simple as data-turbo-progress-bar=true.

nickjj commented 2 years ago

I like that idea, do you think it's worth having this new data attribute default to true if data-turbo-action="advance" is set? It could also allow you to set data-turbo-progress-bar=false if data-turbo-action="advance" is defined to disable the progress bar in case you want to out-opt of it during an advance.

This feels pretty "Rails'y" in the sense of convention over configuration? I suppose a question here to anyone out there is do you have a popular use case where you'd want to advance without the progress bar?

dhh commented 2 years ago

Yeah, I'd say it's fair for this to default to true when we use advance. But I'd love to see this as a general control. Then you could also negate. Like data-turbo-progress-bar=false on an advance to NOT show it.

keithamus commented 2 years ago

Taking a look at this, but on a first pass it seems like it's not so straightforward. The FrameController handles requests coming from <turbo-frame> elements, and it has a different lifecycle to a session.visit, it seems?

It looks like FrameController#requestStarted might be a "good" injection point for showing a progress bar? But by this point we're missing submitter, element, and frame arguments. FrameController#proposeVisitIfNavigatedWithAction has all of those but seems to exist mostly to pass the navigation to session.visit which, again, is a different lifecycle.

Am I totally barking up the wrong tree? Do we want to upgrade any frame requests that want a progress bar to a session.visit? Or do I just conditionally call session.adapter.progress.show() inside FrameController#requestStarted?

leoplct commented 2 years ago

Any updates on this?

When using turbo_frame to update content from a slow page it's really useful to have a progress bar. How can I force the progress bar to shows up?

<%=link_to heavy_contoller_path, data:{turbo_frame: 'results'} do %>
      See results of large query
 <% end %>

<%=turbo_frame_tag 'results' do %>
<% end %>

For the moment I solved using this

// FORCE PROGRSS BAR FOR TURBO FRAMES
document.addEventListener("turbo:before-fetch-request", function(e){
    Turbo.navigator.delegate.adapter.showProgressBar();
})
document.addEventListener("turbo:frame-render", function(e) { // 
    Turbo.navigator.delegate.adapter.progressBar.hide()
})
onEXHovia commented 2 years ago

:+1: it would be a great addition to frames, as a temporary solution:

import * as Turbo from '@hotwired/turbo'

export default () => {
  const adapter = Turbo.navigator.delegate.adapter;
  const progressBar = adapter.progressBar;
  const session = Turbo.session;

  let progressBarTimeout = null;
  document.addEventListener('turbo:before-fetch-request', (event: Event) => {
    const target = event.target;
    if (!(target instanceof HTMLElement)) {
      return;
    }

    if ('true' === target.getAttribute('data-turbo-progress-bar')) {
      if (!progressBarTimeout) {
        progressBar.setValue(0);
      }

      progressBarTimeout = window.setTimeout(() => progressBar.show(), session.progressBarDelay);
    }
  });

  document.addEventListener('turbo:before-fetch-response', () => {
    if (progressBarTimeout) {
      window.clearTimeout(progressBarTimeout);
      progressBar.hide();
      progressBarTimeout = null;
    }
  });
};
Zeneixe commented 1 year ago

I 👍 this idea, it would be a great idea to allow enabling the progress bar via data-attribute

mateuszbialowas commented 1 year ago

Add progress bar and cursor waiting class from tailwind.

I used code from @onEXHovia and @leoplct. Thank you

app/javascript/turbo_progress_bar.js

    const adapter = Turbo.navigator.delegate.adapter;
    const progressBar = adapter.progressBar;
    const session = Turbo.session;

    let progressBarTimeout = null;
    document.addEventListener('turbo:before-fetch-request', function (e) {
        const target = e.target;
        if (!(target instanceof HTMLElement) || target.getAttribute('data-turbo-progress-bar') !== 'true') {
            return;
        }

        if (!progressBarTimeout) {
            progressBar.setValue(0);
        }

        progressBarTimeout = window.setTimeout(() => progressBar.show(), session.progressBarDelay);
        document.body.classList.add('cursor-wait');
    });

    document.addEventListener('turbo:before-fetch-response', () => {
        if (progressBarTimeout) {
            window.clearTimeout(progressBarTimeout);
            progressBar.hide();
            progressBarTimeout = null;
            document.body.classList.remove('cursor-wait');
        }
    });

Import this file in app/javascript/application.js

import "./turbo_progress_bar"

progress_bar_on_loading

zhisme commented 1 year ago

I have also stumbled upon this one and have some turbo frames that are quite heavy and having an option to show progress bar for them is really cool opportunity. However I don't want to make some turbo extensions directly in the project, that feature seems like belonging directly to turbo core library. Are PRs welcome regarding this? I could work on it, if there are people looking for it and maintainers agree that it is a cool idea. So please advise

marcoroth commented 1 year ago

@zhisme I think the general idea of having a data-turbo-progress-bar attribute that you can set on a <turbo-frame> is something that could be added to the core library.

Please feel free to explore this idea! Thank you!

zhisme commented 1 year ago

@marcoroth ready for review

gap777 commented 1 year ago

@zhisme is this completed? Merged? What's the status?

zhisme commented 1 year ago

@gap777 it is on review queue I guess. All work regarding feature is done

gap777 commented 1 year ago

@marcoroth I'd love to have access to this... any idea on when it will ship?

marcoroth commented 1 year ago

Hey @gap777, sadly I have no insight to that.

robinboening commented 1 year ago

thanks for taking this on @zhisme ❤️

no updates regarding a review / merge yet?

zhisme commented 1 year ago

Hi @robinboening no updates, still is on review queue, I guess :upside_down_face:

CONDEVIRSMARK commented 1 year ago

.ͼ6 {color: var(--color-codemirror-syntax-keyword);} .ͼ7 {color: var(--color-codemirror-syntax-comment);} .ͼ8 {color: var(--color-codemirror-matchingbracket-text);} .ͼ9 {color: var(--color-codemirror-syntax-string);} .ͼa {color: var(--color-codemirror-syntax-constant);} .ͼb {color: var(--color-codemirror-syntax-constant);} .ͼc {color: var(--color-codemirror-syntax-entity);} .ͼd {color: var(--color-codemirror-syntax-variable);} .ͼe {color: inherit;} .ͼf {font-weight: bold; color: inherit !important;} .ͼg {color: var(--color-codemirror-syntax-comment);} .ͼh {text-decoration: underline;} .ͼi {font-style: italic;} .ͼj {font-weight: bold;} .ͼk {text-decoration: line-through;} .ͼ5 {background: var(--color-codemirror-bg); color: var(--color-codemirror-text);} .ͼ5 .cm-gutters {background: var(--color-codemirror-gutters-bg); border-right-width: 0;} .ͼ5 .cm-lineNumbers .cm-gutterElement {color: var(--color-codemirror-linenumber-text); padding: 0 16px 0 16px;} .ͼ5 .cm-content {caret-color: var(--color-codemirror-cursor); font-family: ui-monospace, SFMono-Regular, SF Mono, Menlo, Consolas, Liberation Mono, monospace; font-size: 12px; background: var(--color-codemirror-lines-bg); line-height: 20px; padding-top: 8px;} .ͼ5.cm-focused .cm-selectionBackground, .ͼ5 .cm-selectionBackground, .ͼ5 .cm-content ::selection {background-color: var(--color-codemirror-selection-bg, #d7d4f0);} .ͼ5.cm-focused {outline: none;} .ͼ5 .cm-content ::-moz-selection {background-color: var(--color-codemirror-selection-bg, #d7d4f0);} .ͼ5 .cm-activeLine {background-color: var(--color-codemirror-activeline-bg);} .ͼ5 .cm-line {padding-left: 16px;}

pySilver commented 1 year ago

@onEXHovia this workaround covers only GET requests inside the frame at the moment, right? POST form submission inside the frame does not trigger progress bar as I see.

pySilver commented 1 year ago

OK for that case (form submission inside the turbo-frame) one should put data-turbo-progress-bar="true" into the <form> tag.

devacc00 commented 1 year ago

@mateuszbialowas Your solution is nice but after loading is finished progress bar instantly disappear instead of making fast forward to 100% like default behaviour of turbo progress bar.

After digging in turbo source code I came up with solution which fix this problem.

// in config/importmap.rb
pin "turbo_progress_bar"

// in app/javascript/applications.js
import "turbo_progress_bar"

// app/javascript/turbo_progress_bar.js
import * as Turbo from '@hotwired/turbo'

const adapter = Turbo.navigator.delegate.adapter;

document.addEventListener('turbo:before-fetch-request', function(event){
    const target = event.target;
    if (!(target instanceof HTMLElement)) return;

    if (target.getAttribute('data-turbo-action') === 'advance') {
        adapter.formSubmissionStarted(this)
    }
});

document.addEventListener('turbo:before-fetch-response', function(event) {
    adapter.formSubmissionFinished(this);
});
semaperepelitsa commented 12 months ago

Updated for turbo-rails:

// in config/importmap.rb
pin "turbo_progress_bar"

// in app/javascript/applications.js
import "turbo_progress_bar"

// app/javascript/turbo_progress_bar.js
import {Turbo} from '@hotwired/turbo-rails'

const adapter = Turbo.navigator.delegate.adapter;

document.addEventListener('turbo:before-fetch-request', function(event){
  const target = event.target;
  if (!(target instanceof HTMLElement)) return;

  if (target.getAttribute('data-turbo-action') === 'advance') {
    adapter.formSubmissionStarted(this)
  }
});

document.addEventListener('turbo:before-fetch-response', function(event) {
  adapter.formSubmissionFinished(this);
});

// Don't forget to add data-turbo-action="advance" to the turbo-frame:
<turbo-frame id="..." data-turbo-action="advance">
ybakos commented 9 months ago

Does the PR need updated for the latest in Turbo Rails? Or is this still just waiting for review?

zhisme commented 9 months ago

@ybakos yes, the PR https://github.com/hotwired/turbo/pull/840 is waiting to be removed ts files like the upstream branch did. I planning to handle that this month and request another review from maintainers

lirtistan commented 6 months ago

@zhisme any updates here? near may now

zhisme commented 5 months ago

actually the upstream branch went far away, and the PR provided by me needed to be reimplemented from scratch. Many classes are no longer available that were used in the PR. Feel free to use my PR as reference for reimplementing the feature