From 3cf19dd2ea154104658737af593a55756fda3cee Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Sun, 22 Aug 2021 20:38:01 +0100 Subject: [PATCH] Rewrite Button to Typescript (#2984) * Rename Button file * Convert to TS * Add debug warning helper Fires `console.warn`, but only when the forum is in debug mode. Can help to inform extension developers of possible issues with their JS code. * Simplify button content template * Rewrite Button component - Prefer `aria-label` over `title` - Don't duplicate button content to `title` attribute - Warn in debug mode if button has no accessible content - Use modern JS/TS syntax (`||=`, spread, etc) * Update to work with new Button component * Update warning Co-authored-by: Matt Kilgore * Fire warning in `oncreate` * Format * Make Button have extensible Attributes type via generics * Update args type * Update js/src/common/components/Button.tsx Co-authored-by: Matt Kilgore Co-authored-by: David Sevilla Martin Co-authored-by: Alexander Skvortsov --- js/src/common/components/Button.js | 75 ----------- js/src/common/components/Button.tsx | 130 +++++++++++++++++++ js/src/common/components/TextEditorButton.js | 13 +- js/src/common/helpers/fireDebugWarning.ts | 16 +++ 4 files changed, 154 insertions(+), 80 deletions(-) delete mode 100644 js/src/common/components/Button.js create mode 100644 js/src/common/components/Button.tsx create mode 100644 js/src/common/helpers/fireDebugWarning.ts diff --git a/js/src/common/components/Button.js b/js/src/common/components/Button.js deleted file mode 100644 index 2d9bfc83d5..0000000000 --- a/js/src/common/components/Button.js +++ /dev/null @@ -1,75 +0,0 @@ -import Component from '../Component'; -import icon from '../helpers/icon'; -import classList from '../utils/classList'; -import extract from '../utils/extract'; -import extractText from '../utils/extractText'; -import LoadingIndicator from './LoadingIndicator'; - -/** - * The `Button` component defines an element which, when clicked, performs an - * action. - * - * ### Attrs - * - * - `icon` The name of the icon class. If specified, the button will be given a - * 'has-icon' class name. - * - `disabled` Whether or not the button is disabled. If truthy, the button - * will be given a 'disabled' class name, and any `onclick` handler will be - * removed. - * - `loading` Whether or not the button should be in a disabled loading state. - * - * All other attrs will be assigned as attributes on the button element. - * - * Note that a Button has no default class names. This is because a Button can - * be used to represent any generic clickable control, like a menu item. - */ -export default class Button extends Component { - view(vnode) { - const attrs = Object.assign({}, this.attrs); - - attrs.type = attrs.type || 'button'; - - // If a tooltip was provided for buttons without additional content, we also - // use this tooltip as text for screen readers - if (attrs.title && !vnode.children) { - attrs['aria-label'] = attrs.title; - } - - // If given a translation object, extract the text. - if (typeof attrs.title === 'object') { - attrs.title = extractText(attrs.title); - } - - // If nothing else is provided, we use the textual button content as tooltip - if (!attrs.title && vnode.children) { - attrs.title = extractText(vnode.children); - } - - const iconName = extract(attrs, 'icon'); - - const loading = extract(attrs, 'loading'); - if (attrs.disabled || loading) { - delete attrs.onclick; - } - - attrs.className = classList([attrs.className, iconName && 'hasIcon', (attrs.disabled || loading) && 'disabled', loading && 'loading']); - - return ; - } - - /** - * Get the template for the button's content. - * - * @return {*} - * @protected - */ - getButtonContent(children) { - const iconName = this.attrs.icon; - - return [ - iconName && iconName !== true ? icon(iconName, { className: 'Button-icon' }) : '', - children ? {children} : '', - this.attrs.loading ? : '', - ]; - } -} diff --git a/js/src/common/components/Button.tsx b/js/src/common/components/Button.tsx new file mode 100644 index 0000000000..bec7aa1d0b --- /dev/null +++ b/js/src/common/components/Button.tsx @@ -0,0 +1,130 @@ +import type Mithril from 'mithril'; +import Component, { ComponentAttrs } from '../Component'; +import fireDebugWarning from '../helpers/fireDebugWarning'; +import icon from '../helpers/icon'; +import classList from '../utils/classList'; +import extractText from '../utils/extractText'; +import LoadingIndicator from './LoadingIndicator'; + +export interface IButtonAttrs extends ComponentAttrs { + /** + * Class(es) of an optional icon to be rendered within the button. + * + * If provided, the button will gain a `has-icon` class. + */ + icon?: string; + /** + * Disables button from user input. + * + * Default: `false` + */ + disabled?: boolean; + /** + * Show a loading spinner within the button. + * + * If `true`, also disables the button. + * + * Default: `false` + */ + loading?: boolean; + /** + * **DEPRECATED:** Please use the `aria-label` attribute instead. For tooltips, use + * the `` component. + * + * Accessible text for the button. This should always be present if the button only + * contains an icon. + * + * The textual content of this attribute is passed to the DOM element as `aria-label`. + * + * @deprecated + */ + title?: string | Mithril.ChildArray; + /** + * Accessible text for the button. This should always be present if the button only + * contains an icon. + * + * The textual content of this attribute is passed to the DOM element as `aria-label`. + */ + 'aria-label'?: string | Mithril.ChildArray; + /** + * Button type. + * + * Default: `"button"` + * + * @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-type + */ + type?: string; +} + +/** + * The `Button` component defines an element which, when clicked, performs an + * action. + * + * Other attrs will be assigned as attributes on the `; + } + + oncreate(vnode: Mithril.VnodeDOM) { + super.oncreate(vnode); + + const { 'aria-label': ariaLabel } = this.attrs; + + if (!ariaLabel && !extractText(vnode.children) && !this.element?.getAttribute?.('aria-label')) { + fireDebugWarning( + '[Flarum Accessibility Warning] Button has no content and no accessible label. This means that screen-readers will not be able to interpret its meaning and just read "Button". Consider providing accessible text via the `aria-label` attribute. https://web.dev/button-name', + this.element + ); + } + } + + /** + * Get the template for the button's content. + */ + protected getButtonContent(children: Mithril.Children): Mithril.ChildArray { + const iconName = this.attrs.icon; + + return [ + iconName && icon(iconName, { className: 'Button-icon' }), + children && {children}, + this.attrs.loading && , + ]; + } +} diff --git a/js/src/common/components/TextEditorButton.js b/js/src/common/components/TextEditorButton.js index 4f379ef400..fa517e627f 100644 --- a/js/src/common/components/TextEditorButton.js +++ b/js/src/common/components/TextEditorButton.js @@ -1,24 +1,27 @@ +import extractText from '../utils/extractText'; import Button from './Button'; import Tooltip from './Tooltip'; /** * The `TextEditorButton` component displays a button suitable for the text * editor toolbar. + * + * Automatically creates tooltips using the Tooltip component and provided text. + * + * ## Attrs + * - `title` - Tooltip for the button */ export default class TextEditorButton extends Button { view(vnode) { const originalView = super.view(vnode); - // Steal tooltip label from the Button superclass - const tooltipText = originalView.attrs.title; - delete originalView.attrs.title; - - return {originalView}; + return {originalView}; } static initAttrs(attrs) { super.initAttrs(attrs); attrs.className = attrs.className || 'Button Button--icon Button--link'; + attrs.tooltipText = attrs.title; } } diff --git a/js/src/common/helpers/fireDebugWarning.ts b/js/src/common/helpers/fireDebugWarning.ts new file mode 100644 index 0000000000..8c58e79863 --- /dev/null +++ b/js/src/common/helpers/fireDebugWarning.ts @@ -0,0 +1,16 @@ +/** + * Calls `console.warn` with the provided arguments, but only if the forum is in debug mode. + * + * This function is intended to provide warnings to extension developers about issues with + * their extensions that may not be easily noticed when testing, such as accessibility + * issues. + * + * These warnings should be hidden on production forums to ensure webmasters are not + * inundated with do-gooders telling them they have an issue when it isn't something they + * can fix. + */ +export default function fireDebugWarning(...args: Parameters): void { + if (!app.forum.attribute('debug')) return; + + console.warn(...args); +}