Pages Editor: implement Edit Task Form #6981
Conversation
PR UpdateWe can now edit Single Question Tasks. This PR is now ready for review! |
| {/* <button>Delete</button> */} | ||
| </div> | ||
| <div className="input-row"> | ||
| <label className="big">Choices</label> |
There was a problem hiding this comment.
This label isn't linked to a form input.
| <div className="input-row"> | ||
| <label | ||
| className="big" | ||
| htmlFor={`task-${taskKey}-instruction`} |
There was a problem hiding this comment.
This doesn't match the id for the text input.
| <label className="narrow"> | ||
| <input | ||
| type="checkbox" | ||
| checked={required} | ||
| onChange={(e) => { | ||
| setRequired(!!e?.target?.checked); | ||
| }} | ||
| /> | ||
| <span> | ||
| Required | ||
| </span> | ||
| </label> |
There was a problem hiding this comment.
A11y tip: always use for with labels, as explicit labels are better supported by assistive tech, eg. Dragon Naturally Speaking.
w3c/aria-practices#2870
| <ul> | ||
| {answers.map(({ label, next }, index) => ( | ||
| <li | ||
| aria-label={`Choice ${index}`} |
There was a problem hiding this comment.
This label should be on the text input, not the list item.
| <input type="checkbox" /> | ||
| Required | ||
| <div className="input-row"> | ||
| <label className="narrow"> |
There was a problem hiding this comment.
Add htmlFor to this label too.
|
I opened zooniverse/front-end-monorepo#5737 for accessible form labels in the monorepo, but it applies here too. |
| @@ -123,6 +135,8 @@ export default function TasksPage() { | |||
| allTasks={workflow.tasks} | |||
| step={workflow.steps[activeStepIndex]} | |||
| stepIndex={activeStepIndex} | |||
| deleteTask={deleteTask} | |||
| updateTask={updateTask} | |||
| /> | |||
There was a problem hiding this comment.
Stylistic thing to put attributes in alphabetical order
| 'single': 'Single Question', | ||
| 'text': 'Text', | ||
| } | ||
|
|
||
| function EditStepDialog({ | ||
| allTasks = {}, |
There was a problem hiding this comment.
this is getting really nit-picky and very personal style... But I like putting the primary / common noun first. For example, tasksAll and taskUpdate. This makes browsing through the code a bit easier to understand.
| @@ -28,7 +35,9 @@ function EditStepDialog({ | |||
| }; | |||
| }); | |||
|
|
|||
| const title = 'Create a (???) Task' | |||
| const firstTask = allTasks?.[taskKeys?.[0]] | |||
There was a problem hiding this comment.
Same here... I'd personally prefer taskFirst with tasksAll
| @@ -62,13 +71,21 @@ function EditStepDialog({ | |||
| key={`editTaskForm-${taskKey}`} | |||
There was a problem hiding this comment.
camelCase and kebab-case mixing
|
@shaunanoordin - PR looks good with a couple minor changes. I did notice an issue with keyboard usage and hitting pr-review-6981.mov |
shaunanoordin
force-pushed
the
pages-editor-pt11
branch
from
be84e5e to
14a5253
Compare
shaunanoordin
force-pushed
the
pages-editor-pt12
branch
from
7f13b1f to
25cee1e
Compare
|
Thanks Jim and Travis! Required follow up once I get this PR merge train done:
Merging this first and then moving to 6991 |
PR Overview
Part of: Pages Editor MVP project and FEM Lab super-project
Follows #6973
Staging branch URL: https://pr-6981.pfe-preview.zooniverse.org/lab/1982/workflows/editor/3711?env=staging
This PR implements the Edit Task Form (the actual interesting bits inside the Edit Step/Page dialog).
Screenshot: user editing Page P0, Text Task T0
Screenshot: user editing Page P0, Single Question Task T0 with 3 choices
Status
WIPReady for review!Do not merge until 6973 is merged. Currently targets pages-editor-pt11 for reviewing purposes; this will be re-targeted to master when ready.