Fix code review issues

- interfaces go to the top
- fix comma conflation
- value no generic
- optionalised props
- line lints
pull/21833/head
Jorik Schellekens 2020-06-10 14:11:36 +01:00
parent d6a532040e
commit 04d91ae0af
3 changed files with 91 additions and 93 deletions

View File

@ -17,79 +17,7 @@
import React from 'react'; import React from 'react';
import {Key} from '../../../Keyboard'; import {Key} from '../../../Keyboard';
import classnames from 'classnames';
/**
* 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);
}
/** /**
* children: React's magic prop. Represents all children given to the element. * children: React's magic prop. Represents all children given to the element.
@ -116,6 +44,79 @@ interface IAccessibleButtonProps extends React.InputHTMLAttributes<Element> {
ref?: React.Ref<Element>, ref?: React.Ref<Element>,
} }
/**
* 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 = { AccessibleButton.defaultProps = {
element: 'div', element: 'div',
role: 'button', role: 'button',

View File

@ -16,31 +16,28 @@ limitations under the License.
*/ */
import React from "react"; import React from "react";
import PropTypes from 'prop-types';
import createReactClass from 'create-react-class';
import SettingsStore from "../../../settings/SettingsStore"; import SettingsStore from "../../../settings/SettingsStore";
import { _t } from '../../../languageHandler'; import { _t } from '../../../languageHandler';
import ToggleSwitch from "./ToggleSwitch"; import ToggleSwitch from "./ToggleSwitch";
import StyledCheckbox from "./StyledCheckbox"; import StyledCheckbox from "./StyledCheckbox";
interface IProps { interface IProps {
// The setting must be a boolean
name: string, name: string,
level: string, level: string,
roomId?: string, // for per-room settings roomId?: string, // for per-room settings
label?: string, // untranslated label?: string, // untranslated
isExplicit: boolean, isExplicit?: boolean,
// XXX: once design replaces all toggles make this the default // XXX: once design replaces all toggles make this the default
useCheckbox?: boolean, useCheckbox?: boolean,
onChange(checked: boolean): void, onChange?(checked: boolean): void,
} }
interface IState { interface IState {
// XXX: make this generic when the settings store is typed value: boolean;
value: any;
} }
export default class SettingsFlag extends React.Component<IProps, IState> { export default class SettingsFlag extends React.Component<IProps, IState> {
constructor(props: IProps) { constructor(props: IProps) {
super(props); super(props);
@ -64,7 +61,7 @@ export default class SettingsFlag extends React.Component<IProps, IState> {
this.onChange(e.target.checked); this.onChange(e.target.checked);
} }
private save = (val?: any): void => { private save = (val?: boolean): void => {
return SettingsStore.setValue( return SettingsStore.setValue(
this.props.name, this.props.name,
this.props.roomId, this.props.roomId,

View File

@ -15,10 +15,21 @@ See the License for the specific language governing permissions and
limitations under the License. limitations under the License.
*/ */
import React, { EventHandler } from "react"; import React from "react";
import classNames from "classnames"; import classNames from "classnames";
import * as sdk from "../../../index"; 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 // Controlled Toggle Switch element, written with Accessibility in mind
export default ({checked, disabled = false, onChange, ...props}: IProps) => { export default ({checked, disabled = false, onChange, ...props}: IProps) => {
const _onClick = () => { const _onClick = () => {
@ -45,14 +56,3 @@ export default ({checked, disabled = false, onChange, ...props}: IProps) => {
</AccessibleButton> </AccessibleButton>
); );
}; };
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,
};