DEV: Remove INLINE_ONEBOX_* constants

There were two constants here, `INLINE_ONEBOX_LOADING_CSS_CLASS` and `INLINE_ONEBOX_CSS_CLASS` that were both longer than the strings they were DRYing up: `inline-onebox-loading` and `inline-onebox` I normally appreciate constants, but in this case it meant that we had a lot of JS imports resulting in many more lines of code (and CPU cycles spent figuring them out.) It also meant we had an `.erb` file and had to invoke Ruby to create the JS file, which meant the app was harder to port to Ember CLI. I removed the constants. It's less DRY but faster and simpler, and arguably the loss of DRYness is not significant as you can still search for the `inline-onebox-loading` and `inline-onebox` strings easily if you are refactoring.
2020-05-07 16:08:48 -04:00 · 2020-05-07 16:08:48 -04:00 · f9608c0af5
parent 3567a90661
commit f9608c0af5
10 changed files with 48 additions and 108 deletions
--- a/app/assets/javascripts/discourse/app/components/composer-editor.js
+++ b/app/assets/javascripts/discourse/app/components/composer-editor.js
@ -43,10 +43,6 @@ import {
  cacheShortUploadUrl,
  resolveAllShortUrls
 } from "pretty-text/upload-short-url";
-import {
-  INLINE_ONEBOX_LOADING_CSS_CLASS,
-  INLINE_ONEBOX_CSS_CLASS
-} from "pretty-text/context/inline-onebox-css-classes";
 import ENV from "discourse-common/config/environment";

 const REBUILD_SCROLL_MAP_EVENTS = ["composer:resized", "composer:typed-reply"];
@ -968,32 +964,29 @@ export default Component.extend({
          // Inline Oneboxes = `a.inline-onebox-loading` -> `a.inline-onebox`

          let loadedOneboxes = $preview.find(
-            `aside.onebox, a.${LOADING_ONEBOX_CSS_CLASS}, a.${INLINE_ONEBOX_CSS_CLASS}`
+            `aside.onebox, a.${LOADING_ONEBOX_CSS_CLASS}, a.inline-onebox-loading`
          ).length;

-          $preview
-            .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;
+          $preview.find(`a.onebox, a.inline-onebox-loading`).each((_, link) => {
+            const $link = $(link);
+            const text = $link.text();
+            const isInline = $link.attr("class") === "inline-onebox-loading";
+            const m = isInline ? inlineOneboxes : oneboxes;

-              if (loadedOneboxes < this.siteSettings.max_oneboxes_per_post) {
-                if (m[text] === undefined) {
-                  m[text] = [];
-                  loadedOneboxes++;
-                }
-                m[text].push(link);
-              } else {
-                if (m[text] !== undefined) {
-                  m[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 (m[text] !== undefined) {
+                m[text].push(link);
+              } else if (isInline) {
+                $link.removeClass("inline-onebox-loading");
+              }
+            }
+          });

          if (Object.keys(oneboxes).length > 0) {
            this._loadOneboxes(oneboxes);
--- a/app/assets/javascripts/pretty-text-bundle.js
+++ b/app/assets/javascripts/pretty-text-bundle.js
@ -11,6 +11,5 @@
 //= require ./pretty-text/addon/sanitizer
 //= require ./pretty-text/addon/oneboxer
 //= require ./pretty-text/addon/oneboxer-cache
-//= require ./pretty-text/addon/context/inline-onebox-css-classes
 //= require ./pretty-text/addon/inline-oneboxer
 //= require ./pretty-text/addon/upload-short-url
--- a/app/assets/javascripts/pretty-text/addon/context/inline-onebox-css-classes.js.erb
+++ b/app/assets/javascripts/pretty-text/addon/context/inline-onebox-css-classes.js.erb
@ -1,5 +0,0 @@
-export const INLINE_ONEBOX_LOADING_CSS_CLASS =
-  "<%= CookedPostProcessor::INLINE_ONEBOX_LOADING_CSS_CLASS %>";
-
-export const INLINE_ONEBOX_CSS_CLASS =
-  "<%= CookedPostProcessor::INLINE_ONEBOX_CSS_CLASS %>";
--- a/app/assets/javascripts/pretty-text/addon/engines/discourse-markdown/onebox.js
+++ b/app/assets/javascripts/pretty-text/addon/engines/discourse-markdown/onebox.js
@ -1,9 +1,5 @@
 import { lookupCache } from "pretty-text/oneboxer-cache";
 import { cachedInlineOnebox } from "pretty-text/inline-oneboxer";
-import {
-  INLINE_ONEBOX_LOADING_CSS_CLASS,
-  INLINE_ONEBOX_CSS_CLASS
-} from "pretty-text/context/inline-onebox-css-classes";

 const ONEBOX = 1;
 const INLINE = 2;
@ -103,9 +99,9 @@ function applyOnebox(state, silent) {

            if (onebox && onebox.title) {
              text.content = onebox.title;
-              attrs.push(["class", INLINE_ONEBOX_CSS_CLASS]);
+              attrs.push(["class", "inline-onebox"]);
            } else if (!onebox) {
-              attrs.push(["class", INLINE_ONEBOX_LOADING_CSS_CLASS]);
+              attrs.push(["class", "inline-onebox-loading"]);
            }
          }
        }
--- a/app/assets/javascripts/pretty-text/addon/inline-oneboxer.js
+++ b/app/assets/javascripts/pretty-text/addon/inline-oneboxer.js
@ -1,8 +1,3 @@
-import {
-  INLINE_ONEBOX_LOADING_CSS_CLASS,
-  INLINE_ONEBOX_CSS_CLASS
-} from "pretty-text/context/inline-onebox-css-classes";
-
 const _cache = {};

 export function applyInlineOneboxes(inline, ajax, opts) {
@ -27,8 +22,8 @@ export function applyInlineOneboxes(inline, ajax, opts) {
        links.forEach(link => {
          $(link)
            .text(onebox.title)
-            .addClass(INLINE_ONEBOX_CSS_CLASS)
-            .removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS);
+            .addClass("inline-onebox")
+            .removeClass("inline-onebox-loading");
        });
      }
    });
--- a/app/assets/javascripts/pretty-text/addon/white-lister.js
+++ b/app/assets/javascripts/pretty-text/addon/white-lister.js
@ -1,8 +1,3 @@
-import {
-  INLINE_ONEBOX_LOADING_CSS_CLASS,
-  INLINE_ONEBOX_CSS_CLASS
-} from "pretty-text/context/inline-onebox-css-classes";
-
 // to match:
 // abcd
 // abcd[test]
@ -121,8 +116,8 @@ export const DEFAULT_LIST = [
  "a.mention",
  "a.mention-group",
  "a.onebox",
-  `a.${INLINE_ONEBOX_CSS_CLASS}`,
-  `a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`,
+  `a.inline-onebox`,
+  `a.inline-onebox-loading`,
  "a[data-bbcode]",
  "a[name]",
  "a[rel=nofollow]",
--- a/lib/cooked_post_processor.rb
+++ b/lib/cooked_post_processor.rb
@ -4,8 +4,6 @@
 # For example, inserting the onebox content, or image sizes/thumbnails.

 class CookedPostProcessor
-  INLINE_ONEBOX_LOADING_CSS_CLASS = "inline-onebox-loading"
-  INLINE_ONEBOX_CSS_CLASS = "inline-onebox"
  LIGHTBOX_WRAPPER_CSS_CLASS = "lightbox-wrapper"
  LOADING_SIZE = 10
  LOADING_COLORS = 32
@ -518,7 +516,7 @@ class CookedPostProcessor
    oneboxes = {}
    inlineOneboxes = {}

-    Oneboxer.apply(@doc, extra_paths: [".#{INLINE_ONEBOX_LOADING_CSS_CLASS}"]) do |url, element|
+    Oneboxer.apply(@doc, extra_paths: [".inline-onebox-loading"]) do |url, element|
      is_onebox = element["class"] == Oneboxer::ONEBOX_CSS_CLASS
      map = is_onebox ? oneboxes : inlineOneboxes
      skip_onebox = limit <= 0 && !map[url]
@ -724,14 +722,14 @@ class CookedPostProcessor

    if title = inline_onebox&.dig(:title)
      element.children = CGI.escapeHTML(title)
-      element.add_class(INLINE_ONEBOX_CSS_CLASS)
+      element.add_class("inline-onebox")
    end

    remove_inline_onebox_loading_class(element)
  end

  def remove_inline_onebox_loading_class(element)
-    element.remove_class(INLINE_ONEBOX_LOADING_CSS_CLASS)
+    element.remove_class("inline-onebox-loading")
  end

  def is_svg?(img)
--- a/spec/components/cooked_post_processor_spec.rb
+++ b/spec/components/cooked_post_processor_spec.rb
@ -97,10 +97,7 @@ describe CookedPostProcessor do
        cpp.post_process

        expect(cpp.html).to have_tag('a',
-          with: {
-            href: url,
-            class: described_class::INLINE_ONEBOX_CSS_CLASS
-          },
+          with: { href: url, class: "inline-onebox" },
          text: title,
          count: 2
        )
@ -113,9 +110,7 @@ describe CookedPostProcessor do
        )

        expect(cpp.html).to have_tag('a',
-          without: {
-            class: described_class::INLINE_ONEBOX_LOADING_CSS_CLASS
-          },
+          without: { class: "inline-onebox-loading" },
          text: not_oneboxed_url,
          count: 1
        )
@ -131,10 +126,6 @@ describe CookedPostProcessor do
    end

    describe 'when post contains inline oneboxes' do
-      let(:loading_css_class) do
-        described_class::INLINE_ONEBOX_LOADING_CSS_CLASS
-      end
-
      before do
        SiteSetting.enable_inline_onebox_on_all_domains = true
      end
@ -148,12 +139,8 @@ describe CookedPostProcessor do
          cpp.post_process

          expect(cpp.html).to have_tag('a',
-            with: {
-              href: UrlHelper.cook_url(url)
-            },
-            without: {
-              class: loading_css_class
-            },
+            with: { href: UrlHelper.cook_url(url) },
+            without: { class: "inline-onebox-loading" },
            text: topic.title,
            count: 1
          )
@ -163,12 +150,8 @@ describe CookedPostProcessor do
          cpp.post_process

          expect(cpp.html).to have_tag('a',
-            with: {
-              href: UrlHelper.cook_url(url)
-            },
-            without: {
-              class: loading_css_class
-            },
+            with: { href: UrlHelper.cook_url(url) },
+            without: { class: "inline-onebox-loading" },
            text: topic.title,
            count: 1
          )
@ -235,33 +218,21 @@ describe CookedPostProcessor do
          html = cpp.html

          expect(html).to_not have_tag('a',
-            with: {
-              href: url_no_path
-            },
-            without: {
-              class: loading_css_class
-            },
+            with: { href: url_no_path },
+            without: { class: "inline-onebox-loading" },
            text: title
          )

          expect(html).to have_tag('a',
-            with: {
-              href: url_with_path
-            },
-            without: {
-              class: loading_css_class
-            },
+            with: { href: url_with_path },
+            without: { class: "inline-onebox-loading" },
            text: title,
            count: 2
          )

          expect(html).to have_tag('a',
-            with: {
-              href: url_with_query_param
-            },
-            without: {
-              class: loading_css_class
-            },
+            with: { href: url_with_query_param },
+            without: { class: "inline-onebox-loading" },
            text: title,
            count: 1
          )
--- a/test/javascripts/acceptance/composer-onebox-test.js
+++ b/test/javascripts/acceptance/composer-onebox-test.js
@ -1,5 +1,4 @@
 import { acceptance } from "helpers/qunit-helpers";
-import { INLINE_ONEBOX_CSS_CLASS } from "pretty-text/context/inline-onebox-css-classes";

 acceptance("Composer - Onebox", {
  loggedIn: true,
@ -36,10 +35,10 @@ http://www.example.com/has-title.html
        .trim(),
      `
 <p><aside class=\"onebox\"><article class=\"onebox-body\"><h3><a href=\"http://www.example.com/article.html\">An interesting article</a></h3></article></aside><br>
-This is another test <a href=\"http://www.example.com/has-title.html\" class=\"${INLINE_ONEBOX_CSS_CLASS}\">This is a great title</a></p>
+This is another test <a href=\"http://www.example.com/has-title.html\" class=\"inline-onebox\">This is a great title</a></p>
 <p><a href=\"http://www.example.com/no-title.html\" class=\"onebox\" target=\"_blank\">http://www.example.com/no-title.html</a></p>
 <p>This is another test <a href=\"http://www.example.com/no-title.html\" class=\"\">http://www.example.com/no-title.html</a><br>
-This is another test <a href=\"http://www.example.com/has-title.html\" class=\"${INLINE_ONEBOX_CSS_CLASS}\">This is a great title</a></p>
+This is another test <a href=\"http://www.example.com/has-title.html\" class=\"inline-onebox\">This is a great title</a></p>
 <p><aside class=\"onebox\"><article class=\"onebox-body\"><h3><a href=\"http://www.example.com/article.html\">An interesting article</a></h3></article></aside></p>
      `.trim()
    );
--- a/test/javascripts/lib/pretty-text-test.js
+++ b/test/javascripts/lib/pretty-text-test.js
@ -2,7 +2,6 @@ import { buildQuote } from "discourse/lib/quote";
 import Post from "discourse/models/post";
 import PrettyText, { buildOptions } from "pretty-text/pretty-text";
 import { IMAGE_VERSION as v } from "pretty-text/emoji/version";
-import { INLINE_ONEBOX_LOADING_CSS_CLASS } from "pretty-text/context/inline-onebox-css-classes";
 import {
  applyCachedInlineOnebox,
  deleteCachedInlineOnebox
@ -205,7 +204,7 @@ QUnit.test("Links", assert => {

  assert.cooked(
    `Youtube: ${link}`,
-    `<p>Youtube: <a href="${link}" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">${link}</a></p>`,
+    `<p>Youtube: <a href="${link}" class="inline-onebox-loading">${link}</a></p>`,
    "allows links to contain query params"
  );

@ -222,7 +221,7 @@ QUnit.test("Links", assert => {

  assert.cooked(
    "Derpy: http://derp.com?__test=1",
-    `<p>Derpy: <a href="http://derp.com?__test=1" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://derp.com?__test=1</a></p>`,
+    `<p>Derpy: <a href="http://derp.com?__test=1" class="inline-onebox-loading">http://derp.com?__test=1</a></p>`,
    "works with double underscores in urls"
  );

@ -252,7 +251,7 @@ QUnit.test("Links", assert => {

  assert.cooked(
    "Batman: http://en.wikipedia.org/wiki/The_Dark_Knight_(film)",
-    `<p>Batman: <a href="http://en.wikipedia.org/wiki/The_Dark_Knight_(film)" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://en.wikipedia.org/wiki/The_Dark_Knight_(film)</a></p>`,
+    `<p>Batman: <a href="http://en.wikipedia.org/wiki/The_Dark_Knight_(film)" class="inline-onebox-loading">http://en.wikipedia.org/wiki/The_Dark_Knight_(film)</a></p>`,
    "autolinks a URL with parentheses (like Wikipedia)"
  );

@ -264,7 +263,7 @@ QUnit.test("Links", assert => {

  assert.cooked(
    "1. View @eviltrout's profile here: http://meta.discourse.org/u/eviltrout/activity<br/>next line.",
-    `<ol>\n<li>View <span class="mention">@eviltrout</span>\'s profile here: <a href="http://meta.discourse.org/u/eviltrout/activity" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://meta.discourse.org/u/eviltrout/activity</a><br>next line.</li>\n</ol>`,
+    `<ol>\n<li>View <span class="mention">@eviltrout</span>\'s profile here: <a href="http://meta.discourse.org/u/eviltrout/activity" class="inline-onebox-loading">http://meta.discourse.org/u/eviltrout/activity</a><br>next line.</li>\n</ol>`,
    "allows autolinking within a list without inserting a paragraph."
  );

@ -289,8 +288,8 @@ QUnit.test("Links", assert => {
  assert.cooked(
    "http://discourse.org and http://discourse.org/another_url and http://www.imdb.com/name/nm2225369",
    '<p><a href="http://discourse.org">http://discourse.org</a> and ' +
-      `<a href="http://discourse.org/another_url" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://discourse.org/another_url</a> and ` +
-      `<a href="http://www.imdb.com/name/nm2225369" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://www.imdb.com/name/nm2225369</a></p>`,
+      `<a href="http://discourse.org/another_url" class="inline-onebox-loading">http://discourse.org/another_url</a> and ` +
+      `<a href="http://www.imdb.com/name/nm2225369" class="inline-onebox-loading">http://www.imdb.com/name/nm2225369</a></p>`,
    "allows multiple links on one line"
  );