Add Shortcut to the Dev Classifier #3532
Conversation
There was a problem hiding this comment.
Just wondering if the style changes are intended to affect the UI of the button for the whole classifier or if you meant to remove those only when display in the dev classifier. Is there a project in production that is using the shortcut task?
Also could be for a future PR, but the shortcut task is tiny and wouldn't take long to convert to es6.
|
This project that is under review uses the shortcut: https://shortcut-to-classifier.pfe-preview.zooniverse.org/projects/sweenkl/steller-watch/classify/?env=production Styling changes were made because the I can prob write this into ES6 pretty quickly right now. |
|
Oh. Just saw your comment after I pushed. I went ahead and rewrote those two components into ES6 - both the shortcut and the editor attached to it. |
There was a problem hiding this comment.
It doesn't really make sense to have the editor in the shared components folder. Could you move it to classifier/tasks?
I'm seeing some prop type warnings when running the tests for Shortcut.
Also, this is unrelated, but could be fixed with this incoming PR. The editor in the workflow edit page in the lab is shown in a if/else statement which if enabled prevents the project owner from editing both the shortcut and their actual task:
I should be able to edit my survey task and be able to edit the shortcut task.
It could come in a separate PR, but it'd be good to have tests for the editor too.
app/classifier/tasks/shortcut.jsx
Outdated
| }; | ||
|
|
||
| Summary.defaultProps = { | ||
| annotation: null, |
There was a problem hiding this comment.
A default prop of null will throw console warnings if the prop is also required. Either prop can't be required or this needs to be set to a default value.
app/classifier/tasks/shortcut.jsx
Outdated
| <div className="unlinked-shortcut"> | ||
|
|
||
| {options.map((answer, i) => { | ||
| if (answer._key == null) { answer._key = Math.random(); } |
There was a problem hiding this comment.
Use strict equality whenever possible.
app/components/shortcut-editor.jsx
Outdated
| {shortcuts && ( | ||
| <div className="workflow-task-editor-choices"> | ||
| {shortcuts.answers.map((shortcut, index) => { | ||
| if (shortcut._key == null) { shortcut._key = Math.random(); } |
app/components/shortcut-editor.jsx
Outdated
| let nextTaskID; | ||
|
|
||
| if (e.target.checked) { | ||
| const taskCount = Object.keys(this.props.workflow.tasks).length; |
There was a problem hiding this comment.
Uncaught TypeError: Cannot read property 'props' of undefined when trying to use the checkbox button on a test project.
app/components/shortcut-editor.jsx
Outdated
| } | ||
| const handleChange = handleInputChange.bind(this.props.workflow); | ||
| const children = React.Children.map(this.props.children, (child) => { | ||
| React.cloneElement(child); |
There was a problem hiding this comment.
I forgot to return here, is why the original task isn't appearing.
| describe('shortcutRendering', function () { | ||
| let wrapper; | ||
|
|
||
| beforeEach(function () { |
There was a problem hiding this comment.
I think this can just be a before() call. The set of tests in this describe block aren't interacting with the component, just checking the expected render.
| let wrapper; | ||
|
|
||
| beforeEach(function () { | ||
| wrapper = mount(<ShortcutEditor task={task} workflow={workflow} />); |
There was a problem hiding this comment.
shallow or render is probably a better choice to use here since you're only checking the render and don't need the full React lifecycle available. Once we start getting a lot of the code tested, using mount when not actually needed will slow down the test run. Good explanation of the differences here: enzymejs/enzyme#465 (comment)
| }; | ||
|
|
||
| describe('ShortcutEditor', function () { | ||
| describe('shortcutRendering', function () { |
There was a problem hiding this comment.
This can just be describe('rendering', function() { or whatever makes sense for the block of tests. The describe statements should be readable like the it statements.
| }); | ||
| }); | ||
|
|
||
| describe('emptyRendering', function () { |
There was a problem hiding this comment.
same comment about readable describe statement.
| let wrapper; | ||
|
|
||
| beforeEach(function () { | ||
| wrapper = mount(<ShortcutEditor task={emptyTask} workflow={emptyWorkflow} />); |
There was a problem hiding this comment.
Same comment about use of mount.
| describe('emptyRendering', function () { | ||
| let wrapper; | ||
|
|
||
| beforeEach(function () { |
There was a problem hiding this comment.
Same comment about using before here.
| sinon.assert.called(toggleStub); | ||
| }); | ||
|
|
||
| it('should remove the correct shortcut when deleted', function () { |
There was a problem hiding this comment.
This isn't really testing the removal, but rather that the handler is firing. I think it'll be clearer if described like the previous test it('should call remove handler when clicked') or something like that.
| sinon.assert.called(removeStub); | ||
| }); | ||
|
|
||
| it('should add a default shortcut when expanded', function () { |
There was a problem hiding this comment.
Same comment as about about test description.
app/classifier/tasks/shortcut.jsx
Outdated
| </div> | ||
| ); | ||
| } else { | ||
| answer = <div className="answer">No answer</div>; |
There was a problem hiding this comment.
In the dev classifier as well as in a test project, when I click the shortcut option and click done, the summary I see is 'No answer'
app/classifier/tasks/shortcut.jsx
Outdated
| } else { | ||
| delete (this.props.annotation.shortcut); | ||
| this.setState({ index: null }); | ||
| this.props.classification.update('annotations'); |
There was a problem hiding this comment.
FYI, @eatyourgreens asked a question about the use of the classification resource rather than the onChange handler that other tasks use: #3522 (comment)
|
FYI there have been some changes to the shortcut task to make it work with the new task viewer rewrite: #3571 |
wgranger
force-pushed
the
shortcut-to-classifier
branch
from
833af85 to
a5725c4
Compare
|
An issue has been opened on the backend to handle shortcut exports with multiple answers. |
There was a problem hiding this comment.
I have a few more minor changes and questions for the editor. Also, while testing manually, I've been running into this issue, FYI: zooniverse/panoptes#2212
I can get it to work with a couple of edits, but something we should also let project teams who are going to use this know. cc @trouille
| if (this.props.task) { | ||
| const answers = this.props.workflow.tasks[this.props.task.unlinkedTask].answers; | ||
| answers.splice(index, 1); | ||
| if (!answers.length) { |
There was a problem hiding this comment.
I'd prefer an explicit comparison here rather than a truthy value, if (answers.length === 0) should work.
| {children} | ||
| <hr /> | ||
|
|
||
| <label htmlFor="shortcut" title="Shortcut Options to End Classification"> |
There was a problem hiding this comment.
This htmlFor isn't pointing at an id. You'll need to add id="shortcut" on to the input so the browser knows what this is referencing.
| <label htmlFor="shortcut" title="Shortcut Options to End Classification"> | ||
| <AutoSave resource={this.props.workflow}> | ||
| <span className="form-label">Shortcut Option</span>{' '} | ||
| <input type="checkbox" checked={shortcuts != null} onChange={this.toggleShortcut} /> |
There was a problem hiding this comment.
Use strict equality whenever possible in javascript. Loose equality, == and != can have unexpected results because it does type conversion: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness
| return ( | ||
| <div> | ||
|
|
||
| {children} |
There was a problem hiding this comment.
Maybe I'm missing it, but why does the ShortcutEditor need to wrap the TaskEditorComponent?
There was a problem hiding this comment.
We decided to use this Decorator pattern when shortcut was reviewed.
|
|
||
| while (!((nextTaskID != null) && !(nextTaskID in this.props.workflow.tasks))) { | ||
| taskIDNumber += 1; | ||
| nextTaskID = `T${taskCount + taskIDNumber}`; |
There was a problem hiding this comment.
Lines 17-23 look to be identical to addNewTask in the workflow lab page: https://github.com/zooniverse/Panoptes-Front-End/blob/master/app/pages/lab/workflow.cjsx#L525-L541
Is there a reason why you're not just passing addNewTask down as a prop and using that? You can rewrite your handler to call addNewTask as well as the couple of Shortcut specific lines, I think just line 29 and line 31?
There was a problem hiding this comment.
So, if addNewTask is bound to the parent (EditWorkflowPage) and passed down as a prop, then the task prop in the shortcut-editor will change when the function is called (since the parent changes the state of selectedTaskKey) and line 29 in shortcut-editor (this.props.task.unlinkedTask = nextTaskID;) will be set on the shortcut task rather than the original task it needs to be linked to. I tried passing the function itself as a prop (addNewTask={this.addNewTask}), but the functionality on the child component was being wonky.
There was a problem hiding this comment.
Ok, could you make a note about DRYing this up in the classifier rewrite issue #3527? Let's tackle it again later.
| {shortcuts && ( | ||
| <div className="workflow-task-editor-choices"> | ||
| {shortcuts.answers.map((shortcut, index) => { | ||
| if (shortcut._key == null) { shortcut._key = Math.random(); } |
There was a problem hiding this comment.
Use strict equality. See note online 75 about why.
|
Oh yes. I'm really glad you brought that up. I noticed the issues related to that Panoptes PR. Good it is being mentioned here. |
wgranger
force-pushed
the
shortcut-to-classifier
branch
from
f65c260 to
7e18a73
Compare
There was a problem hiding this comment.
Looks good and @trouille informed the project using this that the exports aren't fully functional yet.
I think we conceptually need to revisit the shortcut task in general and I'll make a note of it in the rfc issue. I'm not sure how this is now different from using a multiple task type in a combo and setting the multiple task to end the classification.
Describe your changes.
Adds "shortcut" to the dev classifier and creates tests for the component. Also, some styling was changed to clean up code and use preexisting classes.
Review Checklist
rm -rf node_modules/ && npm installand app works as expected?https://shortcut-to-classifier.pfe-preview.zooniverse.org/
Optional
ChangeListenerorPromiseRenderercomponents with code that updates component state?