Feat alert users with feedback instead of disabling next/save buttons (#68)

* feat: implemented user feedback ErrorAlerts
This commit is contained in:
Raymond Zhou
2022-05-13 12:23:42 -07:00
committed by GitHub
parent b5480beaf8
commit 76dcd1a920
20 changed files with 541 additions and 136 deletions

View File

@@ -9,6 +9,8 @@ import messages from './messages';
/**
* Wrapper for alt-text input and isDecorative checkbox control
* @param {obj} errorProps - props for error handling
* {bool} isValid - are alt-text fields valid for saving?
* @param {bool} isDecorative - is the image decorative?
* @param {func} setIsDecorative - handle isDecorative change event
* @param {func} setValue - update alt-text value
@@ -18,6 +20,7 @@ export const AltTextControls = ({
isDecorative,
setIsDecorative,
setValue,
validation,
value,
// inject
intl,
@@ -28,15 +31,22 @@ export const AltTextControls = ({
</Form.Label>
<Form.Control
className="mt-4.5"
disabled={isDecorative}
floatingLabel={intl.formatMessage(messages.altTextFloatingLabel)}
isInvalid={validation.show}
onChange={hooks.onInputChange(setValue)}
type="input"
value={value}
disabled={isDecorative}
onChange={hooks.onInputChange(setValue)}
floatingLabel={intl.formatMessage(messages.altTextFloatingLabel)}
/>
{validation.show
&& (
<Form.Control.Feedback type="invalid">
<FormattedMessage {...messages.altTextLocalFeedback} />
</Form.Control.Feedback>
)}
<Form.Checkbox
className="mt-4.5 decorative-control-label"
checked={isDecorative}
className="mt-4.5 decorative-control-label"
onChange={hooks.onCheckboxChange(setIsDecorative)}
>
<Form.Label>
@@ -46,10 +56,16 @@ export const AltTextControls = ({
</Form.Group>
);
AltTextControls.propTypes = {
error: PropTypes.shape({
show: PropTypes.bool,
}).isRequired,
isDecorative: PropTypes.bool.isRequired,
value: PropTypes.string.isRequired,
setValue: PropTypes.func.isRequired,
setIsDecorative: PropTypes.func.isRequired,
validation: PropTypes.shape({
show: PropTypes.bool,
}).isRequired,
value: PropTypes.string.isRequired,
// inject
intl: intlShape.isRequired,
};

View File

@@ -19,9 +19,14 @@ describe('AltTextControls', () => {
beforeEach(() => {
props.setValue = jest.fn().mockName('props.setValue');
props.setIsDecorative = jest.fn().mockName('props.setIsDecorative');
props.validation = { show: true };
});
describe('render', () => {
test('snapshot: isDecorative=true', () => {
test('snapshot: isDecorative=true errorProps.showSubmissionError=true', () => {
expect(shallow(<AltTextControls {...props} />)).toMatchSnapshot();
});
test('snapshot: isDecorative=true errorProps.showSubmissionError=false', () => {
props.validation.show = false;
expect(shallow(<AltTextControls {...props} />)).toMatchSnapshot();
});
});

View File

@@ -1,6 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
exports[`AltTextControls render snapshot: isDecorative=true 1`] = `
exports[`AltTextControls render snapshot: isDecorative=true errorProps.showSubmissionError=false 1`] = `
<Form.Group
className="mt-4.5"
>
@@ -17,6 +17,7 @@ exports[`AltTextControls render snapshot: isDecorative=true 1`] = `
className="mt-4.5"
disabled={true}
floatingLabel="Alt Text"
isInvalid={false}
onChange={
Object {
"hooks.onInputChange": [MockFunction props.setValue],
@@ -44,3 +45,58 @@ exports[`AltTextControls render snapshot: isDecorative=true 1`] = `
</Form.Checkbox>
</Form.Group>
`;
exports[`AltTextControls render snapshot: isDecorative=true errorProps.showSubmissionError=true 1`] = `
<Form.Group
className="mt-4.5"
>
<Form.Label
as="h4"
>
<FormattedMessage
defaultMessage="Accessibility"
description="Label title for accessibility section."
id="authoring.texteditor.imagesettingsmodal.accessibilityLabel"
/>
</Form.Label>
<Form.Control
className="mt-4.5"
disabled={true}
floatingLabel="Alt Text"
isInvalid={true}
onChange={
Object {
"hooks.onInputChange": [MockFunction props.setValue],
}
}
type="input"
value="props.value"
/>
<Form.Control.Feedback
type="invalid"
>
<FormattedMessage
defaultMessage="Enter alt text"
description="Message feedback for user below the alt text field."
id="authoring.texteditor.imagesettingsmodal.error.altTextLocalFeedback"
/>
</Form.Control.Feedback>
<Form.Checkbox
checked={true}
className="mt-4.5 decorative-control-label"
onChange={
Object {
"hooks.onCheckboxChange": [MockFunction props.setIsDecorative],
}
}
>
<Form.Label>
<FormattedMessage
defaultMessage="This image is decorative (no alt text required)."
description="Checkbox label for whether or not an image is decorative."
id="authoring.texteditor.imagesettingsmodal.decorativeCheckboxLabel"
/>
</Form.Label>
</Form.Checkbox>
</Form.Group>
`;

View File

@@ -5,18 +5,17 @@ exports[`ImageSettingsModal render snapshot 1`] = `
close={[MockFunction props.close]}
confirmAction={
<Button
disabled={
Object {
"hooks.isSaveDisabled": Object {
"isDecorative": false,
"value": "alternative Taxes",
},
}
}
onClick={
Object {
"hooks.onSaveClick": Object {
"altText": "alternative Taxes",
"altText": Object {
"error": Object {
"dismiss": [MockFunction],
"show": "sHoW",
},
"isDecorative": false,
"value": "alternative Taxes",
},
"dimensions": Object {
"height": 13,
"width": 12,
@@ -39,6 +38,17 @@ exports[`ImageSettingsModal render snapshot 1`] = `
isOpen={false}
title="Image Settings"
>
<ErrorAlert
dismissError={[MockFunction]}
hideHeading={true}
isError="sHoW"
>
<FormattedMessage
defaultMessage="Enter alt text or specify that the image is decorative only."
description="Message presented to user when user attempts to save unaccepted altText configuration."
id="authoring.texteditor.imagesettingsmodal.error.altTextError"
/>
</ErrorAlert>
<Button
onClick={[MockFunction props.returnToSelector]}
size="inline"
@@ -108,6 +118,12 @@ exports[`ImageSettingsModal render snapshot 1`] = `
}
/>
<AltTextControls
error={
Object {
"dismiss": [MockFunction],
"show": "sHoW",
}
}
isDecorative={false}
value="alternative Taxes"
/>

View File

@@ -7,6 +7,8 @@ import * as module from './hooks';
export const state = {
altText: (val) => React.useState(val),
dimensions: (val) => React.useState(val),
showDismissibleError: (val) => React.useState(val),
showSubmissionError: (val) => React.useState(val),
isDecorative: (val) => React.useState(val),
isLocked: (val) => React.useState(val),
local: (val) => React.useState(val),
@@ -119,6 +121,16 @@ export const dimensionLockHooks = () => {
* {func} setWidth - set width
* @param {string} - new width string
* {func} updateDimensions - set dimensions based on state
* {obj} errorProps - props for user feedback error
* {bool} isError - true if dimensions are blank
* {func} setError - sets isError to true
* {func} dismissError - sets isError to false
* {bool} isHeightValid - true if height field is ready to save
* {func} setHeightValid - sets isHeightValid to true
* {func} setHeightNotValid - sets isHeightValid to false
* {bool} isWidthValid - true if width field is ready to save
* {func} setWidthValid - sets isWidthValid to true
* {func} setWidthNotValid - sets isWidthValid to false
*/
export const dimensionHooks = () => {
const [dimensions, setDimensions] = module.state.dimensions(null);
@@ -165,16 +177,50 @@ export const dimensionHooks = () => {
* @param {string} - new alt text
* {bool} isDecorative - is the image decorative?
* {func} setIsDecorative - set isDecorative field
* @param {bool} isDecorative
* {obj} error - error at top of page
* {bool} show - is error being displayed?
* {func} set - set show to true
* {func} dismiss - set show to false
* {obj} validation - local alt text error
* {bool} show - is validation error being displayed?
* {func} set - set validation to true
* {func} dismiss - set validation to false
*/
export const altTextHooks = (savedText) => {
const [value, setValue] = module.state.altText(savedText || '');
const [isDecorative, setIsDecorative] = module.state.isDecorative(false);
const [showDismissibleError, setShowDismissibleError] = module.state.showDismissibleError(false);
const [showSubmissionError, setShowSubmissionError] = module.state.showSubmissionError(false);
const validateAltText = (newVal, newDecorative) => {
if (showSubmissionError) {
if (newVal || newDecorative) {
setShowSubmissionError(false);
}
}
};
return {
value,
setValue,
setValue: (val) => {
setValue(val);
validateAltText(val, null);
},
isDecorative,
setIsDecorative,
setIsDecorative: (decorative) => {
setIsDecorative(decorative);
validateAltText(null, decorative);
},
error: {
show: showDismissibleError,
set: () => setShowDismissibleError(true),
dismiss: () => setShowDismissibleError(false),
},
validation: {
show: showSubmissionError,
set: () => setShowSubmissionError(true),
dismiss: () => setShowSubmissionError(false),
},
};
};
@@ -194,6 +240,25 @@ export const onInputChange = (handleValue) => (e) => handleValue(e.target.value)
*/
export const onCheckboxChange = (handleValue) => (e) => handleValue(e.target.checked);
/**
* checkFormValidation({ altText, isDecorative, onAltTextFail })
* Handle saving the image context to the text editor
* @param {string} altText - image alt text
* @param {bool} isDecorative - is the image decorative?
* @param {func} onAltTextFail - called if alt text validation fails
*/
export const checkFormValidation = ({
altText,
isDecorative,
onAltTextFail,
}) => {
if (!isDecorative && altText === '') {
onAltTextFail();
return false;
}
return true;
};
/**
* onSave({ altText, dimensions, isDecorative, saveToEditor })
* Handle saving the image context to the text editor
@@ -207,27 +272,30 @@ export const onSaveClick = ({
dimensions,
isDecorative,
saveToEditor,
}) => () => saveToEditor({
altText,
dimensions,
isDecorative,
});
/**
* isSaveDisabled(altText)
* Returns true the save button should be disabled (altText is missing and not decorative)
* @param {object} altText - altText hook object
* {bool} isDecorative - is the image decorative?
* {string} value - alt text value
* @return {bool} - should the save button be disabled?
*/
export const isSaveDisabled = (altText) => !altText.isDecorative && (altText.value === '');
}) => () => {
if (module.checkFormValidation({
altText: altText.value,
isDecorative,
onAltTextFail: () => {
altText.error.set();
altText.validation.set();
},
})) {
altText.error.dismiss();
altText.validation.dismiss();
saveToEditor({
altText: altText.value,
dimensions,
isDecorative,
});
}
};
export default {
altText: altTextHooks,
dimensions: dimensionHooks,
isSaveDisabled,
onCheckboxChange,
onInputChange,
onSaveClick,
checkFormValidation,
};

View File

@@ -34,6 +34,8 @@ describe('state values', () => {
};
test('provides altText state value', () => testStateMethod(state.keys.altText));
test('provides dimensions state value', () => testStateMethod(state.keys.dimensions));
test('provides showDismissibleError state value', () => testStateMethod(state.keys.showDismissibleError));
test('provides showSubmissionError state value', () => testStateMethod(state.keys.showSubmissionError));
test('provides isDecorative state value', () => testStateMethod(state.keys.isDecorative));
test('provides isLocked state value', () => testStateMethod(state.keys.isLocked));
test('provides local state value', () => testStateMethod(state.keys.local));
@@ -42,6 +44,9 @@ describe('state values', () => {
});
describe('ImageSettingsModal hooks', () => {
beforeEach(() => {
jest.clearAllMocks();
});
describe('dimensions-related hooks', () => {
describe('getValidDimensions', () => {
it('returns local dimensions if not locked', () => {
@@ -169,12 +174,12 @@ describe('ImageSettingsModal hooks', () => {
state.restore();
});
it('initializes dimension lock hooks with incoming dimension value', () => {
state.mockVal(state.keys.dimensions, reducedDims, state.setState.local);
state.mockVal(state.keys.dimensions, reducedDims);
hook = hooks.dimensionHooks();
expect(hooks.dimensionLockHooks).toHaveBeenCalledWith({ dimensions: reducedDims });
});
test('value is tied to local state', () => {
state.mockVal(state.keys.local, simpleDims, state.setState.local);
state.mockVal(state.keys.local, simpleDims);
hook = hooks.dimensionHooks();
expect(hook.value).toEqual(simpleDims);
});
@@ -200,14 +205,14 @@ describe('ImageSettingsModal hooks', () => {
});
describe('setHeight', () => {
it('sets local height to int value of argument', () => {
state.mockVal(state.keys.local, simpleDims, state.setState.local);
state.mockVal(state.keys.local, simpleDims);
hooks.dimensionHooks().setHeight('23.4');
expect(state.setState.local).toHaveBeenCalledWith({ ...simpleDims, height: 23 });
});
});
describe('setWidth', () => {
it('sets local width to int value of argument', () => {
state.mockVal(state.keys.local, simpleDims, state.setState.local);
state.mockVal(state.keys.local, simpleDims);
hooks.dimensionHooks().setWidth('34.5');
expect(state.setState.local).toHaveBeenCalledWith({ ...simpleDims, width: 34 });
});
@@ -237,17 +242,67 @@ describe('ImageSettingsModal hooks', () => {
});
});
describe('altTextHooks', () => {
it('returns value and isDecorative, along with associated setters', () => {
const value = 'myVAL';
const isDecorative = true;
const showDismissibleError = 'dismiSSiBLE';
const showSubmissionError = 'subMISsion';
beforeEach(() => {
state.mock();
const value = 'myVAL';
const isDecorative = 'IS WE Decorating?';
state.mockVal(state.keys.altText, value, state.setState.altText);
state.mockVal(state.keys.isDecorative, isDecorative, state.setState.isDecorative);
hook = hooks.altTextHooks();
});
afterEach(() => {
state.restore();
});
it('returns value and isDecorative', () => {
state.mockVal(state.keys.altText, value);
state.mockVal(state.keys.isDecorative, isDecorative);
hook = hooks.altTextHooks();
expect(hook.value).toEqual(value);
expect(hook.setValue).toEqual(state.setState.altText);
expect(hook.isDecorative).toEqual(isDecorative);
expect(hook.setIsDecorative).toEqual(state.setState.isDecorative);
});
test('setValue sets value', () => {
state.mockVal(state.keys.altText, value);
hook = hooks.altTextHooks();
hook.setValue(value);
expect(state.setState.altText).toHaveBeenCalledWith(value);
});
test('setIsDecorative sets isDecorative', () => {
state.mockVal(state.keys.altText, value);
hook = hooks.altTextHooks();
hook.setIsDecorative(value);
expect(state.setState.isDecorative).toHaveBeenCalledWith(value);
});
describe('error', () => {
test('show is initialized to false and returns properly', () => {
expect(hook.error.show).toEqual(false);
state.mockVal(state.keys.showDismissibleError, showDismissibleError);
hook = hooks.altTextHooks();
expect(hook.error.show).toEqual(showDismissibleError);
});
test('set sets showDismissibleError to true', () => {
hook.error.set();
expect(state.setState.showDismissibleError).toHaveBeenCalledWith(true);
});
test('dismiss sets showDismissibleError to false', () => {
hook.error.dismiss();
expect(state.setState.showDismissibleError).toHaveBeenCalledWith(false);
});
});
describe('validation', () => {
test('show is initialized to false and returns properly', () => {
expect(hook.validation.show).toEqual(false);
state.mockVal(state.keys.showSubmissionError, showSubmissionError);
hook = hooks.altTextHooks();
expect(hook.validation.show).toEqual(showSubmissionError);
});
test('set sets showSubmissionError to true', () => {
hook.validation.set();
expect(state.setState.showSubmissionError).toHaveBeenCalledWith(true);
});
test('dismiss sets showSubmissionError to false', () => {
hook.validation.dismiss();
expect(state.setState.showSubmissionError).toHaveBeenCalledWith(false);
});
});
});
describe('onInputChange', () => {
@@ -266,31 +321,71 @@ describe('ImageSettingsModal hooks', () => {
expect(onChange).toHaveBeenCalledWith(checked);
});
});
describe('onSaveClick', () => {
it('calls saveToEditor with dimensions, altText, and isDecorative', () => {
const dimensions = simpleDims;
const altText = 'What is this?';
const isDecorative = 'probably';
const saveToEditor = jest.fn();
hooks.onSaveClick({
altText,
dimensions,
isDecorative,
saveToEditor,
})();
expect(saveToEditor).toHaveBeenCalledWith({
altText,
dimensions,
isDecorative,
});
describe('checkFormValidation', () => {
const props = {
onAltTextFail: jest.fn().mockName('onAltTextFail'),
};
beforeEach(() => {
props.altText = '';
props.isDecorative = false;
});
it('calls onAltTextFail when isDecorative is false and altText is an empty string', () => {
hooks.checkFormValidation({ ...props });
expect(props.onAltTextFail).toHaveBeenCalled();
});
it('returns false when isDeocrative is false and altText is an empty string', () => {
expect(hooks.checkFormValidation({ ...props })).toEqual(false);
});
it('returns true when isDecorative is true', () => {
props.isDecorative = true;
expect(hooks.checkFormValidation({ ...props })).toEqual(true);
});
});
describe('isSaveDisabled', () => {
it('returns true iff is not decorative and altText value is empty', () => {
hook = hooks.isSaveDisabled;
expect(hook({ isDecorative: false, value: '' })).toEqual(true);
expect(hook({ isDecorative: false, value: 'test' })).toEqual(false);
expect(hook({ isDecorative: true, value: '' })).toEqual(false);
describe('onSaveClick', () => {
const props = {
altText: {
error: {
show: true,
set: jest.fn(),
dismiss: jest.fn(),
},
validation: {
show: true,
set: jest.fn(),
dismiss: jest.fn(),
},
},
dimensions: simpleDims,
saveToEditor: jest.fn().mockName('saveToEditor'),
};
beforeEach(() => {
props.altText.value = 'What is this?';
props.isDecorative = false;
});
it('calls checkFormValidation', () => {
jest.spyOn(hooks, hookKeys.checkFormValidation);
hooks.onSaveClick({ ...props })();
expect(hooks.checkFormValidation).toHaveBeenCalled();
});
it('calls saveToEditor with dimensions, altText and isDecorative when checkFormValidation is true', () => {
jest.spyOn(hooks, hookKeys.checkFormValidation).mockReturnValueOnce(true);
hooks.onSaveClick({ ...props })();
expect(props.saveToEditor).toHaveBeenCalledWith({
altText: props.altText.value,
dimensions: props.dimensions,
isDecorative: props.isDecorative,
});
});
it('calls dismissError and sets showSubmissionError to false when checkFormValidation is true', () => {
jest.spyOn(hooks, hookKeys.checkFormValidation).mockReturnValueOnce(true);
hooks.onSaveClick({ ...props })();
expect(props.altText.error.dismiss).toHaveBeenCalled();
expect(props.altText.validation.dismiss).toHaveBeenCalled();
});
it('does not call saveEditor when checkFormValidation is false', () => {
jest.spyOn(hooks, hookKeys.checkFormValidation).mockReturnValueOnce(false);
hooks.onSaveClick({ ...props })();
expect(props.saveToEditor).not.toHaveBeenCalled();
});
});
});

View File

@@ -1,16 +1,17 @@
import React from 'react';
import PropTypes from 'prop-types';
import { Button, Image } from '@edx/paragon';
import { ArrowBackIos } from '@edx/paragon/icons';
import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import BaseModal from '../BaseModal';
import AltTextControls from './AltTextControls';
import DimensionControls from './DimensionControls';
import './index.scss';
import hooks from './hooks';
import messages from './messages';
import './index.scss';
import BaseModal from '../BaseModal';
import AltTextControls from './AltTextControls';
import DimensionControls from './DimensionControls';
import ErrorAlert from '../ErrorAlerts/ErrorAlert';
/**
* Modal display wrapping the dimension and alt-text controls for image tags
@@ -34,10 +35,10 @@ export const ImageSettingsModal = ({
const dimensions = hooks.dimensions();
const altText = hooks.altText();
const onSaveClick = hooks.onSaveClick({
saveToEditor,
altText,
dimensions: dimensions.value,
altText: altText.value,
isDecorative: altText.isDecorative,
saveToEditor,
});
return (
<BaseModal
@@ -48,12 +49,18 @@ export const ImageSettingsModal = ({
<Button
variant="primary"
onClick={onSaveClick}
disabled={hooks.isSaveDisabled(altText)}
>
<FormattedMessage {...messages.saveButtonLabel} />
</Button>
)}
>
<ErrorAlert
dismissError={altText.error.dismiss}
hideHeading
isError={altText.error.show}
>
<FormattedMessage {...messages.altTextError} />
</ErrorAlert>
<Button
onClick={returnToSelection}
variant="link"

View File

@@ -17,9 +17,12 @@ jest.mock('./hooks', () => ({
altText: () => ({
value: 'alternative Taxes',
isDecorative: false,
error: {
show: 'sHoW',
dismiss: jest.fn(),
},
}),
onSaveClick: (args) => ({ 'hooks.onSaveClick': args }),
isSaveDisabled: (args) => ({ 'hooks.isSaveDisabled': args }),
}));
describe('ImageSettingsModal', () => {

View File

@@ -59,6 +59,18 @@ export const messages = {
defaultMessage: 'This image is decorative (no alt text required).',
description: 'Checkbox label for whether or not an image is decorative.',
},
// User Feedback Errors
altTextError: {
id: 'authoring.texteditor.imagesettingsmodal.error.altTextError',
defaultMessage: 'Enter alt text or specify that the image is decorative only.',
description: 'Message presented to user when user attempts to save unaccepted altText configuration.',
},
altTextLocalFeedback: {
id: 'authoring.texteditor.imagesettingsmodal.error.altTextLocalFeedback',
defaultMessage: 'Enter alt text',
description: 'Message feedback for user below the alt text field.',
},
};
export default messages;