From 04d91ae0af4bfb6e04090ad75f09ef562da3bbfc Mon Sep 17 00:00:00 2001 From: Jorik Schellekens Date: Wed, 10 Jun 2020 14:11:36 +0100 Subject: [PATCH] Fix code review issues - interfaces go to the top - fix comma conflation - value no generic - optionalised props - line lints --- .../views/elements/AccessibleButton.tsx | 147 +++++++++--------- .../views/elements/SettingsFlag.tsx | 13 +- .../views/elements/ToggleSwitch.tsx | 24 +-- 3 files changed, 91 insertions(+), 93 deletions(-) diff --git a/src/components/views/elements/AccessibleButton.tsx b/src/components/views/elements/AccessibleButton.tsx index ecd4847d0d..ec8e1b250a 100644 --- a/src/components/views/elements/AccessibleButton.tsx +++ b/src/components/views/elements/AccessibleButton.tsx @@ -17,79 +17,7 @@ import React from 'react'; import {Key} from '../../../Keyboard'; - -/** - * AccessibleButton is a generic wrapper for any element that should be treated - * as a button. Identifies the element as a button, setting proper tab - * indexing and keyboard activation behavior. - * - * @param {Object} props react element properties - * @returns {Object} rendered react - */ -export default function AccessibleButton({ - element, - onClick, - children, - kind, - disabled, - inputRef, - className, - ...restProps -}: IProps) { - - const newProps: IAccessibleButtonProps = restProps; - if (!disabled) { - newProps.onClick = onClick, - // We need to consume enter onKeyDown and space onKeyUp - // otherwise we are risking also activating other keyboard focusable elements - // that might receive focus as a result of the AccessibleButtonClick action - // It's because we are using html buttons at a few places e.g. inside dialogs - // And divs which we report as role button to assistive technologies. - // Browsers handle space and enter keypresses differently and we are only adjusting to the - // inconsistencies here - newProps.onKeyDown = (e) => { - if (e.key === Key.ENTER) { - e.stopPropagation(); - e.preventDefault(); - return onClick(e); - } - if (e.key === Key.SPACE) { - e.stopPropagation(); - e.preventDefault(); - } - }, - newProps.onKeyUp = (e) => { - if (e.key === Key.SPACE) { - e.stopPropagation(); - e.preventDefault(); - return onClick(e); - } - if (e.key === Key.ENTER) { - e.stopPropagation(); - e.preventDefault(); - } - } - } - - // Pass through the ref - used for keyboard shortcut access to some buttons - newProps.ref = inputRef; - - newProps.className = (className ? className + " " : "") + "mx_AccessibleButton"; - - if (kind) { - // We apply a hasKind class to maintain backwards compatibility with - // buttons which might not know about kind and break - newProps.className += " mx_AccessibleButton_hasKind mx_AccessibleButton_kind_" + kind; - } - - if (disabled) { - newProps.className += " mx_AccessibleButton_disabled"; - newProps["aria-disabled"] = true; - } - - // React.createElement expects InputHTMLAttributes - return React.createElement(element, restProps, children); -} +import classnames from 'classnames'; /** * children: React's magic prop. Represents all children given to the element. @@ -116,6 +44,79 @@ interface IAccessibleButtonProps extends React.InputHTMLAttributes { ref?: React.Ref, } +/** + * AccessibleButton is a generic wrapper for any element that should be treated + * as a button. Identifies the element as a button, setting proper tab + * indexing and keyboard activation behavior. + * + * @param {Object} props react element properties + * @returns {Object} rendered react + */ +export default function AccessibleButton({ + element, + onClick, + children, + kind, + disabled, + inputRef, + className, + ...restProps +}: IProps) { + + const newProps: IAccessibleButtonProps = restProps; + if (!disabled) { + newProps.onClick = onClick; + // We need to consume enter onKeyDown and space onKeyUp + // otherwise we are risking also activating other keyboard focusable elements + // that might receive focus as a result of the AccessibleButtonClick action + // It's because we are using html buttons at a few places e.g. inside dialogs + // And divs which we report as role button to assistive technologies. + // Browsers handle space and enter keypresses differently and we are only adjusting to the + // inconsistencies here + newProps.onKeyDown = (e) => { + if (e.key === Key.ENTER) { + e.stopPropagation(); + e.preventDefault(); + return onClick(e); + } + if (e.key === Key.SPACE) { + e.stopPropagation(); + e.preventDefault(); + } + }; + newProps.onKeyUp = (e) => { + if (e.key === Key.SPACE) { + e.stopPropagation(); + e.preventDefault(); + return onClick(e); + } + if (e.key === Key.ENTER) { + e.stopPropagation(); + e.preventDefault(); + } + }; + } + + // Pass through the ref - used for keyboard shortcut access to some buttons + newProps.ref = inputRef; + + newProps.className = classnames("mx_AccessibleButton", className); + + if (kind) { + // We apply a hasKind class to maintain backwards compatibility with + // buttons which might not know about kind and break + newProps.className += " mx_AccessibleButton_hasKind mx_AccessibleButton_kind_" + kind; + } + + if (disabled) { + newProps.className += " mx_AccessibleButton_disabled"; + newProps["aria-disabled"] = true; + } + + // React.createElement expects InputHTMLAttributes + return React.createElement(element, restProps, children); +} + AccessibleButton.defaultProps = { element: 'div', role: 'button', diff --git a/src/components/views/elements/SettingsFlag.tsx b/src/components/views/elements/SettingsFlag.tsx index 2ae9bc3d87..ac11e080f6 100644 --- a/src/components/views/elements/SettingsFlag.tsx +++ b/src/components/views/elements/SettingsFlag.tsx @@ -16,31 +16,28 @@ limitations under the License. */ import React from "react"; -import PropTypes from 'prop-types'; -import createReactClass from 'create-react-class'; import SettingsStore from "../../../settings/SettingsStore"; import { _t } from '../../../languageHandler'; import ToggleSwitch from "./ToggleSwitch"; import StyledCheckbox from "./StyledCheckbox"; interface IProps { + // The setting must be a boolean name: string, level: string, roomId?: string, // for per-room settings label?: string, // untranslated - isExplicit: boolean, + isExplicit?: boolean, // XXX: once design replaces all toggles make this the default useCheckbox?: boolean, - onChange(checked: boolean): void, + onChange?(checked: boolean): void, } interface IState { - // XXX: make this generic when the settings store is typed - value: any; + value: boolean; } export default class SettingsFlag extends React.Component { - constructor(props: IProps) { super(props); @@ -64,7 +61,7 @@ export default class SettingsFlag extends React.Component { this.onChange(e.target.checked); } - private save = (val?: any): void => { + private save = (val?: boolean): void => { return SettingsStore.setValue( this.props.name, this.props.roomId, diff --git a/src/components/views/elements/ToggleSwitch.tsx b/src/components/views/elements/ToggleSwitch.tsx index 902538052b..9d092c58a7 100644 --- a/src/components/views/elements/ToggleSwitch.tsx +++ b/src/components/views/elements/ToggleSwitch.tsx @@ -15,10 +15,21 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React, { EventHandler } from "react"; +import React from "react"; import classNames from "classnames"; import * as sdk from "../../../index"; +interface IProps { + // Whether or not this toggle is in the 'on' position. + checked: boolean, + + // Whether or not the user can interact with the switch + disabled: boolean, + + // Called when the checked state changes. First argument will be the new state. + onChange(checked: boolean): void, +}; + // Controlled Toggle Switch element, written with Accessibility in mind export default ({checked, disabled = false, onChange, ...props}: IProps) => { const _onClick = () => { @@ -45,14 +56,3 @@ export default ({checked, disabled = false, onChange, ...props}: IProps) => { ); }; - -interface IProps { - // Whether or not this toggle is in the 'on' position. - checked: boolean, - - // Whether or not the user can interact with the switch - disabled: boolean, - - // Called when the checked state changes. First argument will be the new state. - onChange(checked: boolean): void, -};