DEV: Flexible quote button (PR #15531)

Draft: Really don’t bother looking at this yet! Unless you like looking into those funhouse mirrors, if so be my guest.

GitHub

Feels a little odd conceptually that the mixin calls this._getRequiredQuoteData but _getRequiredQuoteData is only defined in the topic-quote-button component. Same for _noCloseContentEl and _findCooked below. Maybe we should abstract this into one getQuoteData function that each component must have. (And we can have a default implementation in the mixin that posts a console warning if the component is missing its own getQuoteData).

Do you think it would be complicated to strip out the jQuery usage while we’re at this?

Would be good to remove jQuery from the event hooks here as well.

The mixin handles setting up the selection and mouse events for a quote, as well as the more generic parts of the fast edit functionality.

Hmm, should a component/mixin that does more than quoting still be called quote-button?

I wonder if this should wait until after the release? It seems like a relatively sizable/risky change? :thinking:

Not too bad, I was hoping to avoid it but I will do it now that we are delaying this to post-release :slight_smile:

Yep will do

I see what you mean, it is a little bit clunky but these functions really are specific to the quote button of the area that we are quoting from:

// code related to extraction of quote from elements
  //
  // this must be overridden for any other different type
  // of quote buttons that we implement, because everything
  // can have different data needed to form the quote and
  // different CSS classes etc.
  _getRequiredQuoteData($ancestor, requiredData = {}) {
    if (requiredData.postId) {
      return requiredData;
    }
    return { postId: $ancestor.closest(".boxed, .reply").data("post-id") };
  },

  _hasRequiredQuoteData(requiredQuoteData) {
    return requiredQuoteData.postId !== null;
  },

  _noCloseContentEl($ancestor) {
    return $ancestor.closest(".contents").length === 0;
  },

  _noCloseQuotableEl($selectionStart) {
    return $selectionStart.closest(".cooked").length === 0;
  },

  _findCooked($selectedElement) {
    return (
      $selectedElement.find(".cooked")[0] ||
      $selectedElement.closest(".cooked")[0]
    );
  },

Perhaps we just include all of these inside the quote-button mixin as empty methods then throw not implemented errors e.g.

_findCooked($selectedElement) {
  throw new Error("not implemented");
},

The chat usage of this will require further customization than the topic one (I think).

The mixin handles setting up the selection and mouse events for a quote, as well as the more generic parts of the fast edit functionality.

Hmm, should a component/mixin that does more than quoting still be called

@gschlager you make a good point, it does a lot even before this change. Maybe quote-selection would be better? So Quote SelectionMixin.

Not really sure what I have broken but it looks like the tests are just running forever :thinking:

Turns out we don’t want this, at least not for now, so closing.