From f9608c0af5f7b1109117a5aba979acb00c28cf9a Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 7 May 2020 16:08:48 -0400 Subject: [PATCH] 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. --- .../app/components/composer-editor.js | 45 +++++++--------- app/assets/javascripts/pretty-text-bundle.js | 1 - .../context/inline-onebox-css-classes.js.erb | 5 -- .../engines/discourse-markdown/onebox.js | 8 +-- .../pretty-text/addon/inline-oneboxer.js | 9 +--- .../pretty-text/addon/white-lister.js | 9 +--- lib/cooked_post_processor.rb | 8 ++- spec/components/cooked_post_processor_spec.rb | 53 +++++-------------- .../acceptance/composer-onebox-test.js | 5 +- test/javascripts/lib/pretty-text-test.js | 13 +++-- 10 files changed, 48 insertions(+), 108 deletions(-) delete mode 100644 app/assets/javascripts/pretty-text/addon/context/inline-onebox-css-classes.js.erb diff --git a/app/assets/javascripts/discourse/app/components/composer-editor.js b/app/assets/javascripts/discourse/app/components/composer-editor.js index 7d89caa070..7af84831e7 100644 --- 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); diff --git a/app/assets/javascripts/pretty-text-bundle.js b/app/assets/javascripts/pretty-text-bundle.js index a2adade382..62dd6c1439 100644 --- 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 diff --git 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 deleted file mode 100644 index a252fb2880..0000000000 --- a/app/assets/javascripts/pretty-text/addon/context/inline-onebox-css-classes.js.erb +++ /dev/null @@ -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 %>"; diff --git a/app/assets/javascripts/pretty-text/addon/engines/discourse-markdown/onebox.js b/app/assets/javascripts/pretty-text/addon/engines/discourse-markdown/onebox.js index 7358bdaa06..94bd5dd60f 100644 --- 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"]); } } } diff --git a/app/assets/javascripts/pretty-text/addon/inline-oneboxer.js b/app/assets/javascripts/pretty-text/addon/inline-oneboxer.js index 6645d1dac7..a4587eda74 100644 --- 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"); }); } }); diff --git a/app/assets/javascripts/pretty-text/addon/white-lister.js b/app/assets/javascripts/pretty-text/addon/white-lister.js index 8a3a1140f1..88eb29085b 100644 --- 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]", diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index d2e8f4d2d5..d6fb05a308 100644 --- 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) diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index a429d64bee..b62350d2be 100644 --- 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 ) diff --git a/test/javascripts/acceptance/composer-onebox-test.js b/test/javascripts/acceptance/composer-onebox-test.js index 6e047238b0..ab2484d1e9 100644 --- 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(), `


-This is another test This is a great title

+This is another test This is a great title

http://www.example.com/no-title.html

This is another test http://www.example.com/no-title.html
-This is another test This is a great title

+This is another test This is a great title

`.trim() ); diff --git a/test/javascripts/lib/pretty-text-test.js b/test/javascripts/lib/pretty-text-test.js index 8443cccdb6..a5619bcc77 100644 --- 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}`, - `

Youtube: ${link}

`, + `

Youtube: ${link}

`, "allows links to contain query params" ); @@ -222,7 +221,7 @@ QUnit.test("Links", assert => { assert.cooked( "Derpy: http://derp.com?__test=1", - `

Derpy: http://derp.com?__test=1

`, + `

Derpy: http://derp.com?__test=1

`, "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)", - `

Batman: http://en.wikipedia.org/wiki/The_Dark_Knight_(film)

`, + `

Batman: http://en.wikipedia.org/wiki/The_Dark_Knight_(film)

`, "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
next line.", - `
    \n
  1. View @eviltrout\'s profile here: http://meta.discourse.org/u/eviltrout/activity
    next line.
    2. \n
`, + `
    \n
  1. View @eviltrout\'s profile here: http://meta.discourse.org/u/eviltrout/activity
    next line.
    2. \n
`, "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", '

http://discourse.org and ' + - `http://discourse.org/another_url and ` + - `http://www.imdb.com/name/nm2225369

`, + `http://discourse.org/another_url and ` + + `http://www.imdb.com/name/nm2225369

`, "allows multiple links on one line" );