FIX: preview up to 'max_oneboxes_per_post' oneboxes

FIX: preview up to ‘max_oneboxes_per_post’ oneboxes

We were counting all the oneboxes in the DOM instead of just the ones in the preview.

Also refactored the logic to count up to 'max_oneboxes_per_post` instead of down to 0. That also ensured we don’t load 11 oneboxes when the setting is limiting to 10.

diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6
index 9061181..3c204ec 100644
--- a/app/assets/javascripts/discourse/components/composer-editor.js.es6
+++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6
@@ -1026,39 +1026,37 @@ export default Ember.Component.extend({
       Ember.run.debounce(
         this,
         () => {
-          const inlineOneboxes = {};
           const oneboxes = {};
+          const inlineOneboxes = {};
+
+          // Oneboxes = `a.onebox` -> `a.onebox-loading` -> `aside.onebox`
+          // Inline Oneboxes = `a.inline-onebox-loading` -> `a.inline-onebox`
 
-          let oneboxLeft =
-            this.siteSettings.max_oneboxes_per_post -
-            $(
-              `aside.onebox, a.${INLINE_ONEBOX_CSS_CLASS}, a.${LOADING_ONEBOX_CSS_CLASS}`
-            ).length;
+          let loadedOneboxes = $preview.find(
+            `aside.onebox, a.${LOADING_ONEBOX_CSS_CLASS}, a.${INLINE_ONEBOX_CSS_CLASS}`
+          ).length;
 
           $preview
-            .find(`a.${INLINE_ONEBOX_LOADING_CSS_CLASS}, a.onebox`)
-            .each((_index, link) => {
+            .find(`a.onebox, a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`)
+            .each((_, link) => {
               const $link = $(link);
               const text = $link.text();
-
               const isInline =
                 $link.attr("class") === INLINE_ONEBOX_LOADING_CSS_CLASS;
+              const m = isInline ? inlineOneboxes : oneboxes;
 
-              const map = isInline ? inlineOneboxes : oneboxes;
-
-              if (oneboxLeft <= 0) {
-                if (map[text] !== undefined) {
-                  map[text].push(link);
-                } else if (isInline) {
-                  $link.removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS);
+              if (loadedOneboxes < this.siteSettings.max_oneboxes_per_post) {
+                if (m[text] === undefined) {
+                  m[text] = [];
+                  loadedOneboxes++;
                 }
+                m[text].push(link);
               } else {
-                if (!map[text]) {
-                  map[text] = [];
-                  oneboxLeft--;
+                if (m[text] !== undefined) {
+                  m[text].push(link);
+                } else if (isInline) {
+                  $link.removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS);
                 }
-
-                map[text].push(link);
               }
             });
 
@@ -1072,6 +1070,7 @@ export default Ember.Component.extend({
         },
         450
       );
+
       // Short upload urls need resolution
       resolveAllShortUrls(ajax);

GitHub sha: bb127b81

Any reason why not !m[text]?

I was torn on this, but there are 2 reasons I made that change

  • I feel that m[text] === undefined is more clear than !m[text]. Being a “positive” test rather than a “negative” test makes it easier to grasp/understand

  • It’s also the opposite of m[text] !== undefined a few lines after which helps our eyes identify them by their “pattern”