From a88902950afb87d86b4148a52325174e669694af Mon Sep 17 00:00:00 2001 From: Peter Wagenet Date: Thu, 17 Nov 2022 02:55:15 -0800 Subject: [PATCH] DEV: Convert DButton to a Glimmer Component (#17767) This change is intended to be backwards-compatible with all the previous arguments to `DButton`. A deprecation warning will be triggered when a string is passed to the `@action` argument. This kind of action bubbling has been deprecated in Ember for some time, and should be updated to use closure actions. Co-authored-by: Dan Gebhardt Co-authored-by: David Taylor --- .../app/components/bulk-select-all.js | 7 - .../app/components/composer-save-button.hbs | 9 + .../app/components/composer-save-button.js | 13 +- .../discourse/app/components/d-button.js | 200 ++++++++---------- ...r-component-with-deprecated-parent-view.js | 32 +++ .../app/components/login-reply-button.js | 7 - .../app/templates/components/d-button.hbs | 46 ++-- .../integration/components/d-button-test.js | 40 +++- 8 files changed, 207 insertions(+), 147 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/components/bulk-select-all.js create mode 100644 app/assets/javascripts/discourse/app/components/composer-save-button.hbs create mode 100644 app/assets/javascripts/discourse/app/components/glimmer-component-with-deprecated-parent-view.js delete mode 100644 app/assets/javascripts/discourse/app/components/login-reply-button.js diff --git a/app/assets/javascripts/discourse/app/components/bulk-select-all.js b/app/assets/javascripts/discourse/app/components/bulk-select-all.js deleted file mode 100644 index 63814b2e367..00000000000 --- a/app/assets/javascripts/discourse/app/components/bulk-select-all.js +++ /dev/null @@ -1,7 +0,0 @@ -import DButton from "discourse/components/d-button"; - -export default DButton.extend({ - click() { - $("input.bulk-select:not(checked)").click(); - }, -}); diff --git a/app/assets/javascripts/discourse/app/components/composer-save-button.hbs b/app/assets/javascripts/discourse/app/components/composer-save-button.hbs new file mode 100644 index 00000000000..f2c94cf4e4e --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/composer-save-button.hbs @@ -0,0 +1,9 @@ + diff --git a/app/assets/javascripts/discourse/app/components/composer-save-button.js b/app/assets/javascripts/discourse/app/components/composer-save-button.js index 03200017e92..e3b6178c884 100644 --- a/app/assets/javascripts/discourse/app/components/composer-save-button.js +++ b/app/assets/javascripts/discourse/app/components/composer-save-button.js @@ -1,10 +1,9 @@ -import Button from "discourse/components/d-button"; +import Component from "@glimmer/component"; import I18n from "I18n"; import { translateModKey } from "discourse/lib/utilities"; -export default Button.extend({ - classNameBindings: [":btn-primary", ":create", "disableSubmit:disabled"], - translatedTitle: I18n.t("composer.title", { - modifier: translateModKey("Meta+"), - }), -}); +export default class ComposerSaveButton extends Component { + get translatedTitle() { + return I18n.t("composer.title", { modifier: translateModKey("Meta+") }); + } +} diff --git a/app/assets/javascripts/discourse/app/components/d-button.js b/app/assets/javascripts/discourse/app/components/d-button.js index b2f8d2ccb10..275ac30db4e 100644 --- a/app/assets/javascripts/discourse/app/components/d-button.js +++ b/app/assets/javascripts/discourse/app/components/d-button.js @@ -1,161 +1,137 @@ import { inject as service } from "@ember/service"; +import { action } from "@ember/object"; import { empty, equal, notEmpty } from "@ember/object/computed"; -import Component from "@ember/component"; +import GlimmerComponentWithDeprecatedParentView from "discourse/components/glimmer-component-with-deprecated-parent-view"; +import deprecated from "discourse-common/lib/deprecated"; import DiscourseURL from "discourse/lib/url"; import I18n from "I18n"; -import { computed } from "@ember/object"; -import discourseComputed from "discourse-common/utils/decorators"; -export default Component.extend({ - tagName: "button", - // subclasses need this - layoutName: "components/d-button", - form: null, - type: "button", - title: null, - translatedTitle: null, - label: null, - translatedLabel: null, - ariaLabel: null, - ariaExpanded: null, - ariaControls: null, - translatedAriaLabel: null, - forwardEvent: false, - preventFocus: false, - onKeyDown: null, - router: service(), +const ACTION_AS_STRING_DEPRECATION_ARGS = [ + "DButton no longer supports @action as a string. Please refactor to use an closure action instead.", + { id: "discourse.d-button-action-string" }, +]; - isLoading: computed({ - set(key, value) { - this.set("forceDisabled", !!value); - return value; - }, - }), +export default class DButton extends GlimmerComponentWithDeprecatedParentView { + @service router; - classNameBindings: [ - "isLoading:is-loading", - "btnLink::btn", - "btnLink", - "noText", - "btnType", - ], - attributeBindings: [ - "form", - "isDisabled:disabled", - "computedTitle:title", - "computedAriaLabel:aria-label", - "computedAriaExpanded:aria-expanded", - "ariaControls:aria-controls", - "tabindex", - "type", - ], + @notEmpty("args.icon") + btnIcon; - isDisabled: computed("disabled", "forceDisabled", function () { - return this.forceDisabled || this.disabled; - }), + @equal("args.display", "link") + btnLink; - forceDisabled: false, + @empty("computedLabel") + noText; - btnIcon: notEmpty("icon"), + constructor() { + super(...arguments); + if (typeof this.args.action === "string") { + deprecated(...ACTION_AS_STRING_DEPRECATION_ARGS); + } + } - btnLink: equal("display", "link"), + get forceDisabled() { + return !!this.args.isLoading; + } - @discourseComputed("icon", "computedLabel") - btnType(icon, translatedLabel) { - if (icon) { - return translatedLabel ? "btn-icon-text" : "btn-icon"; - } else if (translatedLabel) { + get isDisabled() { + return this.forceDisabled || this.args.disabled; + } + + get btnType() { + if (this.args.icon) { + return this.computedLabel ? "btn-icon-text" : "btn-icon"; + } else if (this.computedLabel) { return "btn-text"; } - }, + } - noText: empty("computedLabel"), - - @discourseComputed("title", "translatedTitle") - computedTitle(title, translatedTitle) { - if (title) { - return I18n.t(title); + get computedTitle() { + if (this.args.title) { + return I18n.t(this.args.title); } - return translatedTitle; - }, + return this.args.translatedTitle; + } - @discourseComputed("label", "translatedLabel") - computedLabel(label, translatedLabel) { - if (label) { - return I18n.t(label); + get computedLabel() { + if (this.args.label) { + return I18n.t(this.args.label); } - return translatedLabel; - }, + return this.args.translatedLabel; + } - @discourseComputed("ariaLabel", "translatedAriaLabel") - computedAriaLabel(ariaLabel, translatedAriaLabel) { - if (ariaLabel) { - return I18n.t(ariaLabel); + get computedAriaLabel() { + if (this.args.ariaLabel) { + return I18n.t(this.args.ariaLabel); } - if (translatedAriaLabel) { - return translatedAriaLabel; + if (this.args.translatedAriaLabel) { + return this.args.translatedAriaLabel; } - }, + } - @discourseComputed("ariaExpanded") - computedAriaExpanded(ariaExpanded) { - if (ariaExpanded === true) { + get computedAriaExpanded() { + if (this.args.ariaExpanded === true) { return "true"; } - if (ariaExpanded === false) { + if (this.args.ariaExpanded === false) { return "false"; } - }, + } + @action keyDown(e) { - if (this.onKeyDown) { + if (this.args.onKeyDown) { e.stopPropagation(); - this.onKeyDown(e); + this.args.onKeyDown(e); } else if (e.key === "Enter") { this._triggerAction(e); - return false; } - }, + } + @action click(event) { return this._triggerAction(event); - }, + } + @action mouseDown(event) { - if (this.preventFocus) { + if (this.args.preventFocus) { event.preventDefault(); } - }, + } _triggerAction(event) { - let { action, route, href } = this; + const { action: actionVal, route, href } = this.args; - if (action || route || href?.length) { - if (action) { - if (typeof action === "string") { - // Note: This is deprecated in new Embers and needs to be removed in the future. - // There is already a warning in the console. - this.sendAction("action", this.actionParam); - } else if (typeof action === "object" && action.value) { - if (this.forwardEvent) { - action.value(this.actionParam, event); + if (actionVal || route || href?.length) { + if (actionVal) { + const { actionParam, forwardEvent } = this.args; + + if (typeof actionVal === "string") { + deprecated(...ACTION_AS_STRING_DEPRECATION_ARGS); + if (this.parentView?.send) { + this.parentView.send(actionVal, actionParam); } else { - action.value(this.actionParam); + throw new Error( + "DButton could not find a target for the action. Use a closure action instead" + ); } - } else if (typeof this.action === "function") { - if (this.forwardEvent) { - action(this.actionParam, event); + } else if (typeof actionVal === "object" && actionVal.value) { + if (forwardEvent) { + actionVal.value(actionParam, event); } else { - action(this.actionParam); + actionVal.value(actionParam); + } + } else if (typeof actionVal === "function") { + if (forwardEvent) { + actionVal(actionParam, event); + } else { + actionVal(actionParam); } } - } - - if (route) { + } else if (route) { this.router.transitionTo(route); - } - - if (href?.length) { + } else if (href?.length) { DiscourseURL.routeTo(href); } @@ -164,5 +140,5 @@ export default Component.extend({ return false; } - }, -}); + } +} diff --git a/app/assets/javascripts/discourse/app/components/glimmer-component-with-deprecated-parent-view.js b/app/assets/javascripts/discourse/app/components/glimmer-component-with-deprecated-parent-view.js new file mode 100644 index 00000000000..b98e67cf27e --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/glimmer-component-with-deprecated-parent-view.js @@ -0,0 +1,32 @@ +import Component from "@glimmer/component"; +import { + CustomComponentManager, + setInternalComponentManager, +} from "@glimmer/manager"; +import EmberGlimmerComponentManager from "@glimmer/component/-private/ember-component-manager"; + +class GlimmerComponentWithParentViewManager extends CustomComponentManager { + create(owner, componentClass, args, environment, dynamicScope) { + const result = super.create(...arguments); + + result.component.parentView = dynamicScope.view; + dynamicScope.view = result.component; + + return result; + } +} + +/** + * This component has a lightly-extended version of Ember's default Glimmer component manager. + * It gives Glimmer components the ability to reference their parent view which can be useful + * when building backwards-compatible versions of components. Any use of the parentView property + * of the component should be considered deprecated. + */ +export default class GlimmerComponentWithDeprecatedParentView extends Component {} + +setInternalComponentManager( + new GlimmerComponentWithParentViewManager( + (owner) => new EmberGlimmerComponentManager(owner) + ), + GlimmerComponentWithDeprecatedParentView +); diff --git a/app/assets/javascripts/discourse/app/components/login-reply-button.js b/app/assets/javascripts/discourse/app/components/login-reply-button.js deleted file mode 100644 index edc71d444d6..00000000000 --- a/app/assets/javascripts/discourse/app/components/login-reply-button.js +++ /dev/null @@ -1,7 +0,0 @@ -import Button from "discourse/components/d-button"; - -export default Button.extend({ - label: "topic.reply.title", - icon: "reply", - action: "showLogin", -}); diff --git a/app/assets/javascripts/discourse/app/templates/components/d-button.hbs b/app/assets/javascripts/discourse/app/templates/components/d-button.hbs index a05a03a3903..bb60dd2a3a6 100644 --- a/app/assets/javascripts/discourse/app/templates/components/d-button.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/d-button.hbs @@ -1,16 +1,36 @@ -{{#if this.isLoading}} - {{~d-icon "spinner" class="loading-icon"~}} -{{else}} - {{#if this.icon}} - {{~d-icon this.icon~}} +{{! template-lint-disable no-down-event-binding }} + diff --git a/app/assets/javascripts/discourse/tests/integration/components/d-button-test.js b/app/assets/javascripts/discourse/tests/integration/components/d-button-test.js index c2aec5178e2..5b2b2351b25 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/d-button-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/d-button-test.js @@ -1,9 +1,10 @@ import { module, test } from "qunit"; import { setupRenderingTest } from "discourse/tests/helpers/component-test"; -import { render, triggerKeyEvent } from "@ember/test-helpers"; +import { click, render, triggerKeyEvent } from "@ember/test-helpers"; import { exists, query } from "discourse/tests/helpers/qunit-helpers"; import I18n from "I18n"; import { hbs } from "ember-cli-htmlbars"; +import ClassicComponent from "@ember/component"; module("Integration | Component | d-button", function (hooks) { setupRenderingTest(hooks); @@ -241,4 +242,41 @@ module("Integration | Component | d-button", function (hooks) { await triggerKeyEvent(".btn", "keydown", "Enter"); assert.strictEqual(this.foo, "bar"); }); + + test("@action function is triggered on click", async function (assert) { + this.set("foo", null); + this.set("action", () => { + this.set("foo", "bar"); + }); + + await render(hbs``); + + await click(".btn"); + + assert.strictEqual(this.foo, "bar"); + }); + + test("@action can sendAction when passed a string", async function (assert) { + this.set("foo", null); + this.set("legacyActionTriggered", () => this.set("foo", "bar")); + + this.classicComponent = ClassicComponent.extend({ + actions: { + myLegacyAction() { + this.legacyActionTriggered(); + }, + }, + }); + + await render( + hbs` + + + ` + ); + + await click(".btn"); + + assert.strictEqual(this.foo, "bar"); + }); });