From f2ecaf29a626fd214a3932d5a2aaed6c37a6a77b Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Tue, 27 Nov 2018 21:23:41 +0000 Subject: [PATCH] Use input widget state to style task inputs (#5091) * Use input widget state to style task inputs Use checkbox/radio button state (active, checked or focussed) to control the style of adjacent task labels. Remove onFocus(), onBlur(), unfocus() and associated tests from TaskInputField. * Remove unused props Remove a ref that's no longer used, and a label prop that didn't seem to be referenced anywhere. Move classname up to the container label. * Convert TaskInputField to a function Convert task input field to a functional component and add React.memo to speed up rendering. Remove onChange() method and update tests. * Fix multiple choice task tests Rewrite tests to use shallow rendering and test props of the generic task component, not nested children. * Fix single answer task tests Rewrite tests to use shallow rendering and test props of the generic task component, not nested children. * Add checked prop Remove shouldInputBeChecked and move responsibility for checked logic up to parent task. Add boolean checked prop. Use defaultChecked instead of checked, so that checkbox state controls the annotation, rather than vice-versa. * Add autoFocus prop Remove shouldInputBeAutoFocussed. Add boolean autoFocus prop instead, and move autofocus logic up to parent tasks. * Fix text highlighter buttons Use the as prop to changed the rendered HTML element. Apply hover styles directly to task input labels. * Restore missing active and focus hover styles Restore styles for active state (input:checked), input checked and focussed, and label hover on checked imputs. * Define common hover styles Move common hover styles into a shared object. * Define constants for styles Define constants for the default, hover and checked themed styles. * Fix linter warnings * Update sinon spy tests Use the BDD syntax from sinon-chai. --- .../TaskInputField/TaskInputField.jsx | 259 +++++++----------- .../TaskInputField/TaskInputField.spec.jsx | 142 +--------- .../tasks/components/TaskInputField/index.js | 2 +- app/classifier/tasks/drawing/index.cjsx | 5 +- .../components/HighlighterButton.jsx | 7 +- app/classifier/tasks/multiple/index.jsx | 5 +- app/classifier/tasks/multiple/index.spec.js | 29 +- app/classifier/tasks/single/index.jsx | 5 +- app/classifier/tasks/single/index.spec.js | 32 ++- 9 files changed, 163 insertions(+), 323 deletions(-) diff --git a/app/classifier/tasks/components/TaskInputField/TaskInputField.jsx b/app/classifier/tasks/components/TaskInputField/TaskInputField.jsx index 1e60385854..d0963c58ec 100644 --- a/app/classifier/tasks/components/TaskInputField/TaskInputField.jsx +++ b/app/classifier/tasks/components/TaskInputField/TaskInputField.jsx @@ -9,185 +9,144 @@ import { pxToRem, zooTheme } from '../../../../theme'; import TaskInputLabel from './components/TaskInputLabel'; import { doesTheLabelHaveAnImage } from './helpers'; -export const StyledTaskInputField = styled.label` - align-items: baseline; - background-color: ${theme('mode', { +const DEFAULT = { + backgroundColor: theme('mode', { dark: zooTheme.colors.darkTheme.background.default, light: zooTheme.colors.lightTheme.background.default - })}; - border: ${theme('mode', { + }), + border: theme('mode', { dark: `2px solid ${zooTheme.colors.darkTheme.font}`, light: '2px solid transparent' - })}; - box-shadow: 1px 1px 2px 0 rgba(0,0,0,0.5); - color: ${theme('mode', { + }), + color: theme('mode', { dark: zooTheme.colors.darkTheme.font, light: zooTheme.colors.lightTheme.font - })}; + }) +}; + +const HOVER = { + gradientTop: theme('mode', { + dark: zooTheme.colors.darkTheme.button.answer.gradient.top, + light: zooTheme.colors.lightTheme.button.answer.gradient.top + }), + gradientBottom: theme('mode', { + dark: zooTheme.colors.darkTheme.button.answer.gradient.bottom, + light: zooTheme.colors.lightTheme.button.answer.gradient.bottom + }), + color: theme('mode', { + dark: zooTheme.colors.darkTheme.font, + light: 'black' + }) +}; + +const CHECKED = { + background: theme('mode', { + dark: zooTheme.colors.teal.mid, + light: zooTheme.colors.teal.mid + }), + border: theme('mode', { + dark: `2px solid ${zooTheme.colors.teal.mid}`, + light: '2px solid transparent' + }), + color: theme('mode', { + dark: zooTheme.colors.darkTheme.font, + light: 'white' + }) +}; + +export const StyledTaskLabel = styled.span` + align-items: baseline; + background-color: ${DEFAULT.backgroundColor}; + border: ${DEFAULT.border}; + box-shadow: 1px 1px 2px 0 rgba(0,0,0,0.5); + color: ${DEFAULT.color}; cursor: pointer; display: flex; margin: ${pxToRem(10)} 0; - padding: ${(props) => { return doesTheLabelHaveAnImage(props.label) ? '0' : '1ch 2ch'; }}; + padding: ${props => (doesTheLabelHaveAnImage(props.label) ? '0' : '1ch 2ch')}; + + &:hover { + background: linear-gradient(${HOVER.gradientTop}, ${HOVER.gradientBottom}); + border-width: 2px; + border-style: solid; + border-left-color: transparent; + border-right-color: transparent; + border-top-color: ${HOVER.gradientTop}; + border-bottom-color: ${HOVER.gradientBottom}; + color: ${HOVER.color}; + } +`; + +export const StyledTaskInputField = styled.label` position: relative; - &:hover, &:focus, &[data-focus=true] { - background: ${theme('mode', { - dark: `linear-gradient( - ${zooTheme.colors.darkTheme.button.answer.gradient.top}, - ${zooTheme.colors.darkTheme.button.answer.gradient.bottom} - )`, - light: `linear-gradient( - ${zooTheme.colors.lightTheme.button.answer.gradient.top}, - ${zooTheme.colors.lightTheme.button.answer.gradient.bottom} - )` - })}; + input { + opacity: 0.01; + position: absolute; + } + + input:focus + ${StyledTaskLabel} { + background: linear-gradient(${HOVER.gradientTop}, ${HOVER.gradientBottom}); border-width: 2px; border-style: solid; border-left-color: transparent; border-right-color: transparent; - border-top-color: ${theme('mode', { - dark: zooTheme.colors.darkTheme.button.answer.gradient.top, - light: zooTheme.colors.lightTheme.button.answer.gradient.top - })}; - border-bottom-color: ${theme('mode', { - dark: zooTheme.colors.darkTheme.button.answer.gradient.bottom, - light: zooTheme.colors.lightTheme.button.answer.gradient.bottom - })}; - color: ${theme('mode', { - dark: zooTheme.colors.darkTheme.font, - light: 'black' - })}; + border-top-color: ${HOVER.gradientTop}; + border-bottom-color: ${HOVER.gradientBottom}; + color: ${HOVER.color}; } - &:active { - background: ${theme('mode', { - dark: `linear-gradient( - ${zooTheme.colors.darkTheme.button.answer.gradient.top}, - ${zooTheme.colors.darkTheme.button.answer.gradient.bottom} - )`, - light: `linear-gradient( - ${zooTheme.colors.lightTheme.button.answer.gradient.top}, - ${zooTheme.colors.lightTheme.button.answer.gradient.bottom} - )` - })}; + input:active + ${StyledTaskLabel} { + background: linear-gradient(${HOVER.gradientTop}, ${HOVER.gradientBottom}); border-width: 2px; border-style: solid; border-color: ${theme('mode', { dark: zooTheme.colors.teal.dark, light: zooTheme.colors.teal.mid })}; - color: ${theme('mode', { - dark: zooTheme.colors.darkTheme.font, - light: 'black' - })}; + color: ${HOVER.color}; } - &.active { - background: ${theme('mode', { - dark: zooTheme.colors.teal.mid, - light: zooTheme.colors.teal.mid - })}; - border: ${theme('mode', { - dark: `2px solid ${zooTheme.colors.teal.mid}`, - light: '2px solid transparent' - })}; - color: ${theme('mode', { - dark: zooTheme.colors.darkTheme.font, - light: 'white' - })} + input:checked + ${StyledTaskLabel} { + background: ${CHECKED.background}; + border: ${CHECKED.border}; + color: ${CHECKED.color} } - &.active:hover, &.active:focus, &.active[data-focus=true] { - background: ${theme('mode', { - dark: zooTheme.colors.teal.mid, - light: zooTheme.colors.teal.mid - })}; + input:focus:checked + ${StyledTaskLabel}, + input:checked + ${StyledTaskLabel}:hover { border: ${theme('mode', { dark: `2px solid ${zooTheme.colors.teal.dark}`, light: `2px solid ${zooTheme.colors.teal.dark}` })}; } - - input { - opacity: 0.01; - position: absolute; - } `; -function shouldInputBeChecked(annotation, index, type) { - if (type === 'radio') { - const toolIndex = annotation._toolIndex || 0; - if (toolIndex) { - return index === toolIndex; - } - return index === annotation.value; - } - - if (type === 'checkbox') { - return (annotation.value && annotation.value.length > 0) ? annotation.value.includes(index) : false; - } - - return false; -} - -function shouldInputBeAutoFocused(annotation, index, name, type) { - if (type === 'radio' && name === 'drawing-tool') { - return index === 0; - } - - return index === annotation.value; -} - -export class TaskInputField extends React.Component { - constructor() { - super(); - this.unFocus = this.unFocus.bind(this); - } - - onChange(e) { - this.unFocus(); - this.props.onChange(e); - } - - onFocus() { - if (this.field) this.field.dataset.focus = true; - } - - onBlur() { - this.unFocus(); - } - - unFocus() { - if (this.field) this.field.dataset.focus = false; - } - - render() { - return ( - - { this.field = node; }} - className={this.props.className} - label={this.props.label} - data-focus={false} - > - - - - - ); - } +export function TaskInputField(props) { + return ( + + + + + + + + + ); } TaskInputField.defaultProps = { + autoFocus: false, + checked: false, className: '', label: '', labelIcon: null, @@ -198,21 +157,13 @@ TaskInputField.defaultProps = { }; TaskInputField.propTypes = { - annotation: PropTypes.shape({ - _key: PropTypes.number, - task: PropTypes.string, - value: PropTypes.oneOfType([ - PropTypes.arrayOf(PropTypes.number), // mulitple choice - PropTypes.number, // single choice - PropTypes.arrayOf(PropTypes.object), // drawing task - PropTypes.object // null - ]) - }).isRequired, + autoFocus: PropTypes.bool, + checked: PropTypes.bool, className: PropTypes.string, index: PropTypes.number.isRequired, label: PropTypes.string, labelIcon: PropTypes.oneOfType([PropTypes.node, PropTypes.object]), - labelStatus: PropTypes.oneOfType([PropTypes.node, PropTypes.object]), + labelStatus: PropTypes.oneOfType([PropTypes.node, PropTypes.object]), name: PropTypes.string, onChange: PropTypes.func, theme: PropTypes.string, @@ -223,4 +174,4 @@ const mapStateToProps = state => ({ theme: state.userInterface.theme }); -export default connect(mapStateToProps)(TaskInputField); +export default connect(mapStateToProps)(React.memo(TaskInputField)); diff --git a/app/classifier/tasks/components/TaskInputField/TaskInputField.spec.jsx b/app/classifier/tasks/components/TaskInputField/TaskInputField.spec.jsx index b145265a0f..59346a898e 100644 --- a/app/classifier/tasks/components/TaskInputField/TaskInputField.spec.jsx +++ b/app/classifier/tasks/components/TaskInputField/TaskInputField.spec.jsx @@ -12,8 +12,8 @@ import sinon from 'sinon'; import { TaskInputField, StyledTaskInputField } from './TaskInputField'; import { mockReduxStore, radioTypeAnnotation } from '../../testHelpers'; -describe('TaskInputField', function() { - describe('render', function() { +describe('TaskInputField', function () { + describe('render', function () { let wrapper; before(function () { wrapper = mount(, mockReduxStore); @@ -39,150 +39,24 @@ describe('TaskInputField', function() { wrapper.setProps({ className: 'active' }); expect(wrapper.props().className).to.include('active'); }); - }) + }); - describe('onChange method', function() { - let onChangeSpy; - let unFocusSpy; + describe('onChange method', function () { let onChangePropsSpy; let wrapper; - before(function() { - onChangeSpy = sinon.spy(TaskInputField.prototype, 'onChange'); - unFocusSpy = sinon.spy(TaskInputField.prototype, 'unFocus'); + before(function () { onChangePropsSpy = sinon.spy(); wrapper = mount(, mockReduxStore); }); - afterEach(function() { - onChangeSpy.resetHistory(); - unFocusSpy.resetHistory(); + afterEach(function () { onChangePropsSpy.resetHistory(); }); - after(function() { - onChangeSpy.restore(); - unFocusSpy.restore(); - }); - - it('should call onChange when the on change event is fired', function() { - wrapper.find('input').simulate('change'); - expect(onChangeSpy.calledOnce).to.be.true; - }); - - it('should call onFocus in the onChange method', function() { - wrapper.find('input').simulate('change'); - expect(unFocusSpy.calledOnce).to.be.true; - }); - - it('should call props.onChange in the onChange method', function() { + it('should call onChange when the on change event is fired', function () { wrapper.find('input').simulate('change'); - expect(onChangePropsSpy.calledOnce).to.be.true; - }); - }); - - describe('onFocus method', function() { - let wrapper; - let onFocusSpy; - before(function() { - onFocusSpy = sinon.spy(TaskInputField.prototype, 'onFocus'); - wrapper = mount(, mockReduxStore); - }); - - afterEach(function() { - onFocusSpy.resetHistory(); - }); - - after(function() { - onFocusSpy.restore(); - }); - - it('should call onFocus when the on focus event fires', function() { - wrapper.find('input').simulate('focus'); - expect(onFocusSpy.calledOnce).to.be.true; - }); - - // This isn't working. Can data attributes update in tests? - // it('should set the data-focus attribute to true if this.field is defined', function() { - // wrapper.find('input').simulate('focus'); - // wrapper.update(); - // expect(wrapper.instance().field).to.exist; - // expect(wrapper.find(StyledTaskInputField).props()['data-focus']).to.be.true; - // }); - - // it('should not set the data-focus attribute to true if this.field is not defined', function() { - // wrapper.instance().field = null; - // wrapper.find('input').simulate('focus'); - // expect(wrapper.instance().field).to.not.exist; - // expect(wrapper.find(StyledTaskInputField).props()['data-focus']).to.be.false; - // }); - }); - - describe('onBlur method', function() { - let wrapper; - let unFocusSpy; - let onBlurSpy; - before(function() { - onBlurSpy = sinon.spy(TaskInputField.prototype, 'onBlur'); - unFocusSpy = sinon.spy(TaskInputField.prototype, 'unFocus'); - wrapper = mount(, mockReduxStore); - }); - - afterEach(function() { - onBlurSpy.resetHistory(); - unFocusSpy.resetHistory(); - }); - - afterEach(function() { - onBlurSpy.restore(); - unFocusSpy.restore(); - }); - - it('should call onBlur when the on blur event fires', function() { - wrapper.find('input').simulate('blur'); - expect(onBlurSpy.calledOnce).to.be.true; - }); - - it('should call unFocus in the onBlur method', function() { - wrapper.find('input').simulate('blur'); - expect(unFocusSpy.calledOnce).to.be.true; + expect(onChangePropsSpy).to.have.been.calledOnce; }); }); - - // describe.only('unFocus method', function() { - // let wrapper; - // let unFocusSpy; - // before(function () { - // unFocusSpy = sinon.spy(TaskInputField.prototype, 'unFocus'); - // wrapper = mount(, mockReduxStore); - // }); - - // afterEach(function () { - // unFocusSpy.resetHistory(); - // }); - - // afterEach(function () { - // unFocusSpy.restore(); - // }); - - // it('should set the data-focus attribute to true if this.field is defined', function() { - // wrapper.instance().onFocus(); // set data-field focus to true - // wrapper.update(); - // wrapper.instance().unFocus(); - // wrapper.update(); - // expect(wrapper.instance().field).to.exist; - // expect(wrapper.find(StyledTaskInputField).props()['data-focus']).to.be.false; - // }); - - // it('should not set the data-focus attribute to true if this.field is not defined', function() { - // wrapper.instance().onFocus(); // set data-field focus to true - // wrapper.update(); - // console.log(wrapper.debug()) - // wrapper.instance().field = null; - // expect(wrapper.instance().field).to.not.exist; - // wrapper.instance().unFocus(); - // wrapper.update(); - // expect(wrapper.find(StyledTaskInputField).props()['data-focus']).to.be.true; - // }); - // }); }); diff --git a/app/classifier/tasks/components/TaskInputField/index.js b/app/classifier/tasks/components/TaskInputField/index.js index 31ae8368a8..a919cb3127 100644 --- a/app/classifier/tasks/components/TaskInputField/index.js +++ b/app/classifier/tasks/components/TaskInputField/index.js @@ -1,2 +1,2 @@ -export { default, StyledTaskInputField } from './TaskInputField'; +export { default, StyledTaskInputField, StyledTaskLabel } from './TaskInputField'; diff --git a/app/classifier/tasks/drawing/index.cjsx b/app/classifier/tasks/drawing/index.cjsx index 362de8747e..26ab63210c 100644 --- a/app/classifier/tasks/drawing/index.cjsx +++ b/app/classifier/tasks/drawing/index.cjsx @@ -97,10 +97,13 @@ module.exports = createReactClass tool._key ?= Math.random() count = (true for mark in @props.annotation.value when mark.tool is i).length translation = @props.translation.tools[i] + checked = i is (@props.annotation._toolIndex ? 0)
0) ? annotation.value.includes(i) : false; answers.push( ); + summary = shallow(); }); it('should render without crashing', function () { @@ -127,7 +130,7 @@ describe('MultipleChoiceSummary', function () { }); it('should return "No answer" when an empty annotation is provided', function () { - summary = mount(); + summary = shallow(); const answer = summary.find('.answer'); expect(answer.text()).to.equal('No answer'); }); diff --git a/app/classifier/tasks/single/index.jsx b/app/classifier/tasks/single/index.jsx index e76a359e2d..9e445134ba 100644 --- a/app/classifier/tasks/single/index.jsx +++ b/app/classifier/tasks/single/index.jsx @@ -30,11 +30,14 @@ export default class SingleChoiceTask extends React.Component { if (!answer._key) { answer._key = Math.random(); } + const checked = i === annotation.value; answers.push( , mockReduxStore); + wrapper = shallow(, mockReduxStore); }); it('should render without crashing', function () { @@ -24,12 +25,12 @@ describe('SingleChoiceTask', function () { }); it('should have a question', function () { - const question = wrapper.find('.question'); - expect(question.hostNodes()).to.have.lengthOf(1); + const question = wrapper.find(GenericTask).prop('question'); + expect(question).to.equal(radioTypeTask.question); }); it('should have answers', function () { - expect(wrapper.find('TaskInputField')).to.have.lengthOf(radioTypeTask.answers.length); + expect(wrapper.find(GenericTask).prop('answers')).to.have.lengthOf(radioTypeTask.answers.length); }); }); @@ -45,7 +46,7 @@ describe('SingleChoiceTask', function () { }); beforeEach(function() { - wrapper = mount( + wrapper = shallow( ); + summary = shallow(); }); it('should render without crashing', function () { @@ -130,13 +132,13 @@ describe('SingleChoiceSummary', function () { }); it('should have the correct answer label when the value if falsy (i.e. 0)', function () { - summary = mount(); + summary = shallow(); const answers = summary.find('.answer'); expect(answers.text()).to.not.equal('No answer'); }); it('should return "No answer" when annotation is null', function () { - summary = mount(); + summary = shallow(); const answers = summary.find('.answer'); expect(answers.text()).to.equal('No answer'); });