Allow changing the submitter text during form submission

[afcapel]

This change introduces a new optional data-turbo-submitting-text attribute that can be set on submit elements (inputs or buttons) in Turbo forms.

<form action="/" method="post">
  <input type="submit" value="Save" data-turbo-submitting-text="Saving...">
</form>

When the form is in a submmiting state Turbo will change the submitter content -the input value for inputs, and the innerHTML for buttons- with the data-turbo-submitting-text, and restore the original value when the submission ends.

This is a common requirement in many apps, to style the form submitting state and give feedback to the user that a click is already being processed.

[seanpdoyle]

Is this inspired by its rails/ujs predecessor? That attribute's name was [data-disable-with]. Is there a reason that this attribute name diverges from the others ([data-method] to [data-turbo-method], [data-confirm] to [data-turbo-confirm], etc.)?

[kevinmcconnell]

LGTM! Just 2 minor questions.

[kevinmcconnell]

Minor thing, but setSubmittingText and resetSubmittingText are quite similar. I wonder if it would simplify things to use single method, like replaceSubmitterText that sets the new content and returns the previous content.

Then setSubmittingText becomes:

this.originalSubmitText = this.replaceSubmitterText(this.submittingText);

and resetSubmittingText becomes:

this.replaceSubmitterText(this.originalSubmitText)

The guard clause would mean it's still conditional on there being some submit text configured.

Could have setSubmittingText and resetSubmittingText be little wrappers that call the common method, or just inline them.

[afcapel]

Gave that try, the whole change becomes:

  setSubmittingText() {
    this.originalSubmitText = this.replaceSubmitterText(this.submittingText)
  }

  resetSubmittingText() {
    this.replaceSubmitterText(this.originalSubmitText)
    this.originalSubmitText = undefined
  }

  replaceSubmitterText(replacement: string | undefined | null) {
    if (!this.submitter || !replacement) return

    let originalValue

    if (this.submitter.matches("button")) {
      originalValue = this.submitter.innerHTML
      this.submitter.innerHTML = replacement
    } else if (this.submitter.matches("input")) {
      const input = this.submitter as HTMLInputElement
      originalValue = input.value
      input.value = replacement
    }

    return originalValue
  }

That works and removes a bit of duplication, but in practice it feels that the new replaceSubmitterText is harder to grasp, and a tiny bit longer, than the two original methods:

  setSubmittingText() {
    if (!this.submitter || !this.submittingText) return

    if (this.submitter.matches("button")) {
      this.originalSubmitText = this.submitter.innerHTML
      this.submitter.innerHTML = this.submittingText
    } else if (this.submitter.matches("input")) {
      const input = this.submitter as HTMLInputElement
      this.originalSubmitText = input.value
      input.value = this.submittingText
    }
  }

  resetSubmittingText() {
    if (!this.submitter || !this.originalSubmitText) return

    if (this.submitter.matches("button")) {
      this.submitter.innerHTML = this.originalSubmitText
    } else if (this.submitter.matches("input")) {
      const input = this.submitter as HTMLInputElement
      input.value = this.originalSubmitText
    }
  }

[kevinmcconnell]

Personally I like the one that removes the duplication :) I don't have very strong feelings about it though.

[kevinmcconnell]

Is there a race condition here, or is the assert.equal guaranteed to happen before the next event?

[afcapel]

I've added some delay to the submit response so the test picks up the changing test.

[kevinmcconnell]

It would be nice if we could check the values in the event handler, so that we don't have to think about the timing of it.

[afcapel]

Is this inspired by its rails/ujs predecessor? That attribute's name was [data-disable-with]. Is there a reason that this attribute name diverges from the others ([data-method] to [data-turbo-method], [data-confirm] to [data-turbo-confirm], etc.)?

@seanpdoyle it's inspired [data-disable-with], but both are slightly different. [data-disable-with] not only works with submitter buttons, but also with full forms and remote links. Its main point is to disable the annotated element during an XHR request.

But in Turbo we're already disabling the submitter on every form submission, regardless of any data attributes. I feel that if the attribute name was data-turbo-disable-with, it would give the impression that it's a drop-in replacement for [data-disable-with], even when they're functionally different, and that you need the attribute to disable a button on form submission, which is not the case.

[marcoroth]

I kinda agree that a data-*-with attribute would be more "familiar" given how they were previously used. What if we used something like data-turbo-submits-with?

[dhh]

Great to see this come to Turbo. Agree with @marcoroth on familiar naming using data-turbo-submits-with to both differentiate from the fact that there's no extra disabling, but connects with what we had, and reads a little nicer.