From 6fa9b99ef5f0c64ab6248d626a48fb07eae24e0d Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Fri, 14 Sep 2018 09:45:31 -0700 Subject: [PATCH 1/2] refactor(toggle): less race conditions and works in FF --- src/ui/generic/Toggle.css | 77 +++++++++++++++++++------------------- src/ui/generic/Toggle.tsx | 79 +++++++++++++-------------------------- 2 files changed, 64 insertions(+), 92 deletions(-) diff --git a/src/ui/generic/Toggle.css b/src/ui/generic/Toggle.css index 207c19b..d9b9dba 100644 --- a/src/ui/generic/Toggle.css +++ b/src/ui/generic/Toggle.css @@ -1,52 +1,53 @@ .toggle { - -webkit-appearance: none; - border: 0; - width: 2.4rem; - padding: 0.25rem; + background: none; + border: none; + cursor: inherit; + display: block; + outline: inherit; + padding: 0; + position: relative; + width: 2rem; + z-index: 0; } -.toggle, -.toggle:focus { - background-color: transparent; - box-shadow: none; -} +.toggle__bar { + background-color: var(--secondary); -.toggle:disabled { - background-color: transparent; -} + border-radius: 2rem; -.toggle::-webkit-slider-runnable-track { - height: 1rem; border-radius: 1rem; cursor: pointer; -} -.toggle--off::-webkit-slider-runnable-track { - background-color: var(--secondary); - opacity: 0.8; -} -.toggle--on::-webkit-slider-runnable-track { - background-color: var(--primary-opacity-2); -} -.toggle::-webkit-slider-thumb { - -webkit-appearance: none; - width: 1rem; + top: 2px; + left: 0px; + height: 1rem; - border-radius: 1rem; - margin-right: -0.125rem; - background-color: var(--secondary); + width: 100%; + + position: absolute; + z-index: 0; } -.toggle--off::-webkit-slider-thumb { - background-color: var(--text-muted); - opacity: 0.2; + +.toggle__bar--active { + background-color: rgba(28, 126, 214, 0.2); } -.toggle--off::-webkit-slider-thumb:active { - background-color: var(--primary); - opacity: 0.5; + +.toggle__knob { + background-color: var(--text-muted); + opacity: 0.8; + border-radius: 0.5rem; + + display: block; + + height: 1rem; + width: 16px; + margin-top: 2px; + + position: relative; + z-index: 1; } -.toggle--on::-webkit-slider-thumb { + +.toggle__knob--active { background-color: var(--primary); -} -.toggle--on::-webkit-slider-thumb:active { - opacity: 0.7; + transform: translate3d(18px, 0, 0); } diff --git a/src/ui/generic/Toggle.tsx b/src/ui/generic/Toggle.tsx index 8a00c3b..2091b16 100644 --- a/src/ui/generic/Toggle.tsx +++ b/src/ui/generic/Toggle.tsx @@ -30,75 +30,46 @@ export class Toggle extends React.PureComponent { public render(): JSX.Element | null { const value = this.state.value === undefined ? this.props.value : this.state.value - // console.log({ value, stateValue: this.state.value, propsValue: this.props.value }) + return ( - + > + + + ) } - // Track mousedown state to avoid a click triggering both the change and click events (and toggling the value - // twice). Also track whether the value changed during mousedown so that clicking and releasing on one side - // will toggle it. - private mouseDown = false - private changedDuringMouseDown = false - private onMouseDown: React.MouseEventHandler = e => { - this.mouseDown = true - this.changedDuringMouseDown = false - } - - private onChange: React.FormEventHandler = e => { - const value = e.currentTarget.valueAsNumber === 1 - if (this.mouseDown) { - this.changedDuringMouseDown = true - this.setState({ value }) - } else { - this.onToggle(value) - } - } - - private onMouseUp: React.MouseEventHandler = e => { - if (!this.mouseDown) { + private onClick: React.FormEventHandler = e => { + if (this.props.disabled) { return } - this.mouseDown = false - // Clicking and releasing entirely on one side will toggle the current value. - let value = e.currentTarget.valueAsNumber === 1 - if (!this.changedDuringMouseDown) { - const rect = e.currentTarget.getBoundingClientRect() - const mouseOverElement = - rect.left <= e.pageX && - rect.left + rect.width >= e.pageX && - rect.top <= e.pageY && - rect.top + rect.height >= e.pageY - if (!mouseOverElement) { - return + this.setState( + ({ value }) => ({ value: !value }), + () => { + this.onToggle(this.state.value!) } - value = !value - } - this.setState({ value: undefined }, () => this.onToggle(value)) + ) } private onToggle(value: boolean): void { - if (value !== !!this.props.value) { - if (this.props.onToggle) { - this.props.onToggle(value) - } + if (value !== !!this.props.value && this.props.onToggle) { + this.props.onToggle(value) } } } From 66d6222b5903a9a2f28c2b1bf1be9413dc4ff5b5 Mon Sep 17 00:00:00 2001 From: Chris Wendt Date: Thu, 4 Oct 2018 19:18:10 -0700 Subject: [PATCH 2/2] feat: replace extension drop-downs with toggles BREAKING CHANGE: ExtensionPrimaryActionButton is now called ExtensionToggle --- .../ExtensionConfigureButtonDropdown.tsx | 257 ------------------ .../ExtensionPrimaryActionButton.tsx | 90 ------ src/extensions/ExtensionToggle.tsx | 106 ++++++++ src/extensions/manager/ExtensionCard.tsx | 4 +- src/settings.ts | 20 +- src/ui/generic/Toggle.css | 4 +- src/ui/generic/Toggle.tsx | 41 +-- 7 files changed, 139 insertions(+), 383 deletions(-) delete mode 100644 src/extensions/ExtensionConfigureButtonDropdown.tsx delete mode 100644 src/extensions/ExtensionPrimaryActionButton.tsx create mode 100644 src/extensions/ExtensionToggle.tsx diff --git a/src/extensions/ExtensionConfigureButtonDropdown.tsx b/src/extensions/ExtensionConfigureButtonDropdown.tsx deleted file mode 100644 index ac8639e..0000000 --- a/src/extensions/ExtensionConfigureButtonDropdown.tsx +++ /dev/null @@ -1,257 +0,0 @@ -import * as React from 'react' -import { ButtonDropdown, DropdownMenu, DropdownToggle } from 'reactstrap' -import DropdownItem from 'reactstrap/lib/DropdownItem' -import { from, Subject, Subscribable, Subscription } from 'rxjs' -import { catchError, map, mapTo, startWith, switchMap, tap } from 'rxjs/operators' -import { ExtensionsProps } from '../context' -import { asError, ErrorLike, isErrorLike } from '../errors' -import { - ConfigurationCascadeProps, - ConfigurationSubject, - ConfiguredSubjectOrError, - Settings, - subjectLabel, -} from '../settings' -import { ConfiguredExtension, isExtensionAdded, isExtensionEnabled } from './extension' - -const LOADING: 'loading' = 'loading' - -interface ExtensionConfigureDropdownItemState { - /** The operation's status: null when done or not started, 'loading', or an error. */ - operationResultOrError: typeof LOADING | null | ErrorLike -} - -/** An item in the {@link ExtensionConfigureButton} dropdown menu. */ -export class ExtensionConfigureDropdownItem< - S extends ConfigurationSubject, - C extends Settings -> extends React.PureComponent< - { - /** The extension that this button is for. */ - extension: ConfiguredExtension - - /** The configuration subject that this item modifies extension settings for. */ - subject: ConfiguredSubjectOrError - - disabled?: boolean - confirm?: () => boolean - operation: ( - extension: ConfiguredExtension, - subject: ConfiguredSubjectOrError - ) => Subscribable - onUpdate: () => void - onComplete: () => void - } & ExtensionsProps, - ExtensionConfigureDropdownItemState -> { - public state: ExtensionConfigureDropdownItemState = { operationResultOrError: null } - - private clicks = new Subject() - private subscriptions = new Subscription() - - public componentDidMount(): void { - this.subscriptions.add( - this.clicks - .pipe( - switchMap(() => - from(this.props.operation(this.props.extension, this.props.subject)).pipe( - mapTo(null), - tap(() => this.props.onComplete()), - catchError(error => [asError(error) as ErrorLike]), - map(c => ({ operationResultOrError: c } as ExtensionConfigureDropdownItemState)), - tap(() => this.props.onUpdate()), - startWith({ operationResultOrError: LOADING }) - ) - ) - ) - .subscribe(stateUpdate => this.setState(stateUpdate), error => console.error(error)) - ) - } - - public componentWillUnmount(): void { - this.subscriptions.unsubscribe() - } - - public render(): JSX.Element | null { - return ( - - {this.props.children} -
- {isErrorLike(this.state.operationResultOrError) && ( - - Error - - )} -
- - ) - } - - private onClick: React.MouseEventHandler = () => { - if (!this.props.confirm || this.props.confirm()) { - this.clicks.next() - } - } -} - -interface Props - extends ConfigurationCascadeProps, - ExtensionsProps { - /** The extension that this dropdown is for. */ - extension: ConfiguredExtension - - /** The configuration subject that this dropdown modifies extension settings for. */ - subject: ConfiguredSubjectOrError - - /** Class name applied to the button element. */ - buttonClassName?: string - - /* The button label. */ - children: React.ReactFragment - - /** Whether to show the caret on the dropdown toggle. */ - caret?: boolean - - /** - * Called to confirm the primary action. If the callback returns false, the action is not - * performed. - */ - confirm?: () => boolean - - /** Called when the component performs an update that requires the parent component to refresh data. */ - onUpdate: () => void -} - -interface State { - dropdownOpen: boolean -} - -/** - * Displays a button with a dropdown menu for enabling/disabling the extension. - * - * For simplicity, the menu is only intended to expose the most common extension configuration actions for the - * current user. For example, it does not expose actions to configure the extension for all users (in global - * settings) or for an organization's members. To make those changes, the user needs to manually edit global or - * organization settings. - */ -export class ExtensionConfigureButtonDropdown< - S extends ConfigurationSubject, - C extends Settings -> extends React.PureComponent, State> { - public state: State = { - dropdownOpen: false, - } - - public render(): JSX.Element | null { - // Configuration subjects other than this.props.subject for which the extension is added in settings. - const otherSubjectsWithExtensionAdded = - this.props.configurationCascade.subjects && !isErrorLike(this.props.configurationCascade.subjects) - ? this.props.configurationCascade.subjects - .filter(a => a.subject.id !== this.props.subject.subject.id) - .filter(subject => isExtensionAdded(subject.settings, this.props.extension.id)) - : [] - - return ( - - - {this.props.children} - - - {subjectLabel(this.props.subject.subject)} settings: - - Enable extension - - - Disable extension - - {// Hide "Remove extension" button when the extension is present in other lower-precedence - // subjects' settings, because in that case, removing the extension from user settings - // would just fall back to the lower-precedence settings, which would be unexpected to the - // user. To handle these cases, the user must manually edit settings. - otherSubjectsWithExtensionAdded.length === 0 ? ( - - Remove extension - - ) : ( - <> - - subject.__typename === 'Org') - .map(({ subject }) => subjectLabel(subject)) - .join(', ')} - > - - {otherSubjectsWithExtensionAdded.some(({ subject }) => subject.__typename === 'Site') - ? 'Default: enabled for everyone' - : 'Default: enabled for organization'} - - - )} - - - ) - } - - private toggle = () => { - this.setState(prevState => ({ dropdownOpen: !prevState.dropdownOpen })) - } - - private enableExtensionForSubject = ( - extension: ConfiguredExtension, - subject: ConfiguredSubjectOrError - ) => - this.props.extensions.context.updateExtensionSettings(subject.subject.id, { - extensionID: extension.id, - enabled: true, - }) - - private disableExtensionForSubject = ( - extension: ConfiguredExtension, - subject: ConfiguredSubjectOrError - ) => - this.props.extensions.context.updateExtensionSettings(subject.subject.id, { - extensionID: extension.id, - enabled: false, - }) - - private removeExtensionForSubject = ( - extension: ConfiguredExtension, - subject: ConfiguredSubjectOrError - ) => - this.props.extensions.context.updateExtensionSettings(subject.subject.id, { - extensionID: extension.id, - remove: true, - }) - - private onComplete = () => this.setState({ dropdownOpen: false }) -} diff --git a/src/extensions/ExtensionPrimaryActionButton.tsx b/src/extensions/ExtensionPrimaryActionButton.tsx deleted file mode 100644 index 723c92d..0000000 --- a/src/extensions/ExtensionPrimaryActionButton.tsx +++ /dev/null @@ -1,90 +0,0 @@ -import * as React from 'react' -import { ExtensionsProps } from '../context' -import { isErrorLike } from '../errors' -import { ConfigurationCascadeProps, ConfigurationSubject, Settings } from '../settings' -import { ConfiguredExtension, confirmAddExtension, isExtensionAdded } from './extension' -import { ExtensionAddButton } from './ExtensionAddButton' -import { ExtensionConfigureButtonDropdown } from './ExtensionConfigureButtonDropdown' - -interface Props - extends ConfigurationCascadeProps, - ExtensionsProps { - /** The extension that this element is for. */ - extension: ConfiguredExtension - - disabled?: boolean - - /** Class name applied to this element. */ - className?: string - - /** Class name applied to this element when it is an "Add" button. */ - addClassName?: string - - /** Called when the component performs an update that requires the parent component to refresh data. */ - onUpdate: () => void -} - -/** - * Displays the primary action for an extension. - * - * - "Add" if the extension is not yet added and can be added. - * - "Configure (icon)" dropdown menu in all other cases. - */ -export class ExtensionPrimaryActionButton< - S extends ConfigurationSubject, - C extends Settings -> extends React.PureComponent> { - public render(): JSX.Element | null { - if (this.props.configurationCascade.subjects === null) { - return null - } - if (isErrorLike(this.props.configurationCascade.subjects)) { - // TODO: Show error. - return null - } - - // Only operate on the highest precedence settings, for simplicity. - const subjects = this.props.configurationCascade.subjects - if (subjects.length === 0) { - return null - } - const highestPrecedenceSubject = subjects[subjects.length - 1] - if (!highestPrecedenceSubject || !highestPrecedenceSubject.subject.viewerCanAdminister) { - return null - } - - if ( - this.props.configurationCascade.subjects.every(s => !isExtensionAdded(s.settings, this.props.extension.id)) - ) { - return ( - - Add - - ) - } - return ( -
- - - -
- ) - } - - public confirm = () => confirmAddExtension(this.props.extension.id, this.props.extension.manifest) -} diff --git a/src/extensions/ExtensionToggle.tsx b/src/extensions/ExtensionToggle.tsx new file mode 100644 index 0000000..c34623e --- /dev/null +++ b/src/extensions/ExtensionToggle.tsx @@ -0,0 +1,106 @@ +import { last } from 'lodash-es' +import * as React from 'react' +import { EMPTY, from, Subject, Subscription } from 'rxjs' +import { switchMap } from 'rxjs/operators' +import { ExtensionsProps } from '../context' +import { isErrorLike } from '../errors' +import { ConfigurationCascadeProps, ConfigurationSubject, extractErrors, Settings } from '../settings' +import { Toggle } from '../ui/generic/Toggle' +import { ConfiguredExtension, confirmAddExtension, isExtensionAdded, isExtensionEnabled } from './extension' + +interface Props + extends ConfigurationCascadeProps, + ExtensionsProps { + /** The extension that this element is for. */ + extension: ConfiguredExtension + + disabled?: boolean + + /** Class name applied to this element. */ + className?: string + + /** Class name applied to this element when it is an "Add" button. */ + addClassName?: string + + /** Called when the component performs an update that requires the parent component to refresh data. */ + onUpdate: () => void +} + +/** + * Displays a toggle button for an extension. + */ +export class ExtensionToggle extends React.PureComponent< + Props +> { + private toggles = new Subject() + private subscriptions = new Subscription() + + public componentDidMount(): void { + this.subscriptions.add( + this.toggles + .pipe( + switchMap(enabled => { + if (this.props.configurationCascade.subjects === null) { + return EMPTY + } + if (isErrorLike(this.props.configurationCascade.subjects)) { + // TODO: Show error. + return EMPTY + } + + // Only operate on the highest precedence settings, for simplicity. + const subjects = this.props.configurationCascade.subjects + if (subjects.length === 0) { + return EMPTY + } + const highestPrecedenceSubject = subjects[subjects.length - 1] + if (!highestPrecedenceSubject || !highestPrecedenceSubject.subject.viewerCanAdminister) { + return EMPTY + } + + if ( + !isExtensionAdded(this.props.configurationCascade.merged, this.props.extension.id) && + !confirmAddExtension(this.props.extension.id, this.props.extension.manifest) + ) { + return EMPTY + } + + return from( + this.props.extensions.context.updateExtensionSettings(highestPrecedenceSubject.subject.id, { + extensionID: this.props.extension.id, + enabled, + }) + ) + }) + ) + .subscribe() + ) + } + + public componentWillUnmount(): void { + this.subscriptions.unsubscribe() + } + + public render(): JSX.Element | null { + const cascade = extractErrors(this.props.configurationCascade) + const subject = isErrorLike(cascade) + ? undefined + : last(cascade.subjects.filter(subject => isExtensionAdded(subject.settings, this.props.extension.id))) + const state = subject && { + state: subject.settings.extensions ? subject.settings.extensions[this.props.extension.id] : false, + name: subject.subject.__typename, + } + + const onToggle = (enabled: boolean) => { + this.toggles.next(enabled) + } + + return ( + + ) + } +} diff --git a/src/extensions/manager/ExtensionCard.tsx b/src/extensions/manager/ExtensionCard.tsx index 61ec7e4..476f9dc 100644 --- a/src/extensions/manager/ExtensionCard.tsx +++ b/src/extensions/manager/ExtensionCard.tsx @@ -7,7 +7,7 @@ import { ConfigurationCascadeProps, ConfigurationSubject, Settings } from '../.. import { LinkOrSpan } from '../../ui/generic/LinkOrSpan' import { ConfiguredExtension, isExtensionAdded, isExtensionEnabled } from '../extension' import { ExtensionConfigurationState } from '../ExtensionConfigurationState' -import { ExtensionPrimaryActionButton } from '../ExtensionPrimaryActionButton' +import { ExtensionToggle } from '../ExtensionToggle' interface Props extends ConfigurationCascadeProps, @@ -88,7 +88,7 @@ export class ExtensionCard e
  • {props.subject && (props.subject.viewerCanAdminister ? ( - { +export interface ConfigurationCascade< + S extends ConfigurationSubject = ConfigurationSubject, + C extends Settings = Settings +> { /** * The settings for each subject in the cascade, from lowest to highest precedence. */ @@ -153,6 +156,21 @@ export function gqlToCascade return cascade } +/** Converts a ConfigurationCascadeOrError to a ConfigurationCascade, returning the first error it finds. */ +export function extractErrors( + c: ConfigurationCascadeOrError +): ConfigurationCascade | ErrorLike { + if (c.subjects === null || isErrorLike(c.subjects)) { + return new Error('Subjects was ' + c.subjects) + } else if (c.merged === null || isErrorLike(c.merged)) { + return new Error('Merged was ' + c.merged) + } else if (c.subjects.find(isErrorLike)) { + return new Error('One of the subjects was ' + c.subjects.find(isErrorLike)) + } else { + return c as ConfigurationCascade + } +} + /** * Deeply merges the settings without modifying any of the input values. The array is ordered from lowest to * highest precedence in the merge. diff --git a/src/ui/generic/Toggle.css b/src/ui/generic/Toggle.css index d9b9dba..80e2671 100644 --- a/src/ui/generic/Toggle.css +++ b/src/ui/generic/Toggle.css @@ -1,9 +1,9 @@ .toggle { background: none; border: none; - cursor: inherit; + cursor: pointer; display: block; - outline: inherit; + outline: none !important; padding: 0; position: relative; width: 2rem; diff --git a/src/ui/generic/Toggle.tsx b/src/ui/generic/Toggle.tsx index 2091b16..1c156a8 100644 --- a/src/ui/generic/Toggle.tsx +++ b/src/ui/generic/Toggle.tsx @@ -20,56 +20,35 @@ interface Props { className?: string } -interface State { - value: boolean | undefined -} - /** A toggle switch input component. */ -export class Toggle extends React.PureComponent { - public state: State = { value: undefined } - +export class Toggle extends React.PureComponent { public render(): JSX.Element | null { - const value = this.state.value === undefined ? this.props.value : this.state.value + const onClick = () => { + if (this.props.onToggle) { + this.props.onToggle(!this.props.value) + } + } return ( ) } - - private onClick: React.FormEventHandler = e => { - if (this.props.disabled) { - return - } - - this.setState( - ({ value }) => ({ value: !value }), - () => { - this.onToggle(this.state.value!) - } - ) - } - - private onToggle(value: boolean): void { - if (value !== !!this.props.value && this.props.onToggle) { - this.props.onToggle(value) - } - } }