Skip to content

Commit

Permalink
Pages Editor: misc updates for internal review (zooniverse#7105)
Browse files Browse the repository at this point in the history
* pages-editor-pt25: add checkIsPFEWorkflow() function

* rename canStepBranch() to checkCanStepBranch()

* DataManager: add PFE workflow warning

* WorkflowSettingsPage: fix missing configuration.multi_image_mode dropdown

* Whoops! Accidentally disabled Limited Branching Rule during a demo.

* WIP: update documentation for Limited Branching Rule

* WIP Limited Branching Rule: can now open New Task Dialog even if step is branching

* WIP Limited Branching Rule: can now convert ONE multiple-type task (if many) into a single-type task

* Limited Branching Rule (complete): newly added Question Tasks are single-type if step isn't branching, multiple-type otherwise

* Limited Branching Rule: cleanup. Complete.

* Next Page feature: prevent pages from linking to itself

* EditStepDialog: reword Add Task button

* Add better handling for Tasks that don't exist

* Documentation: add comment on why deleteTask() no longer checks for task's existence

* Design: increase font size of several elements on TasksPage

* Add experimental container style. Makes 'next step' arrow for branching tasks appear outside the container

* StepItem: fix experimental container style when there are 0 answers, or too many answers

* Minor documentation

* Feedback: fix gradient on select element, in Safari

* Feedback: update comments on checkStepBranch() about multiple branching tasks

* Feeback: TasksPage's deleteTask safety check and comments updated
  • Loading branch information
shaunanoordin authored Jun 7, 2024
1 parent a698f8c commit b19edc1
Show file tree
Hide file tree
Showing 16 changed files with 238 additions and 114 deletions.
20 changes: 19 additions & 1 deletion app/pages/lab-pages-editor/DataManager.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { useEffect, useMemo, useState } from 'react';
import PropTypes from 'prop-types';
import apiClient from 'panoptes-client/lib/api-client';
import { WorkflowContext } from './context.js';
import checkIsPFEWorkflow from './helpers/checkIsPFEWorkflow.js';

function DataManager({
// key: to ensure DataManager renders FRESH (with states reset) whenever workflowId changes, use <DataManager key={workflowId} ... />
Expand All @@ -30,6 +31,7 @@ function DataManager({
status: 'ready'
});
const [updateCounter, setUpdateCounter] = useState(0); // Number of updates so far, only used to trigger useMemo.
const isPFEWorkflow = checkIsPFEWorkflow(apiData.workflow);

// Fetch workflow when the component loads for the first time.
// See notes about 'key' prop, to ensure states are reset:
Expand Down Expand Up @@ -129,11 +131,27 @@ function DataManager({
{apiData.status === 'error' && (
<div className="status-banner error">ERROR: could not fetch data</div>
)}
{children}
{isPFEWorkflow ? <PFEWorkflowWarning /> : children }
</WorkflowContext.Provider>
);
}

/*
At the moment, we don't fully support PFE workflows in the Pages Editor.
Reason: the "autocleanup" functionality (see cleanupTasksAndSteps()) would
annihilate all the tasks in a workflow with no steps (i.e. PFE workflows).
We'd need to prompt users to convert from a PFE workflow to an FEM workflow
first before allowing them
*/
function PFEWorkflowWarning() {
return (
<div className="pfe-workflow-warning">
<p>Sorry, but the new workflow editor doesn't support traditional workflows.</p>
<p>Please check back with us later, as we improve the workflow editor.</p>
</div>
);
}

DataManager.propTypes = {
children: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.node),
Expand Down
17 changes: 8 additions & 9 deletions app/pages/lab-pages-editor/components/TasksPage/TasksPage.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useRef, useState } from 'react'
import { useWorkflowContext } from '../../context.js';
import canStepBranch from '../../helpers/canStepBranch.js';
import checkCanStepBranch from '../../helpers/checkCanStepBranch.js';
import createStep from '../../helpers/createStep.js';
import createTask from '../../helpers/createTask.js';
import getNewStepKey from '../../helpers/getNewStepKey.js';
Expand Down Expand Up @@ -100,8 +100,9 @@ export default function TasksPage() {
}

function deleteTask(taskKey) {
// First check: does the task exist?
if (!workflow || !taskKey || !workflow?.tasks?.[taskKey]) return;
if (!workflow) return;
// NOTE: don't check if task actually exists before deleting it.
// This is useful if a Step somehow refers to a Task that doesn't exist.

// Second check: is this the only task in the step?
const activeStepTaskKeys = workflow.steps?.[activeStepIndex]?.[1]?.taskKeys || [];
Expand Down Expand Up @@ -262,14 +263,12 @@ export default function TasksPage() {

// Limited Branching Rule:
// 0. a Step can only have 1 branching task (single answer question task)
// 1. if a Step has a branching task, it can't have any other tasks.
// 2. if a Step already has at least one task, any added question task must be a multiple answer question task.
// 3. if a Step already has many tasks, any multiple answer question task can't be transformed into a single answer question task.
// 1. if a Step has a branching task, it can't have ANOTHER BRANCHING TASK.
// 2. if a Step already has at least one BRANCHING task, any added question task must be a multiple answer question task.
// 3. if a Step already has many MULTIPLE ANSWER QUESTION tasks AND NO SINGLE ANSWER QUESTION TASK, ONLY ONE multiple answer question task CAN be transformed into a single answer question task.
const activeStep = workflow?.steps?.[activeStepIndex]
const enforceLimitedBranchingRule = {
stepHasBranch: !!canStepBranch(activeStep, workflow?.tasks),
stepHasOneTask: activeStep?.[1]?.taskKeys?.length > 0,
stepHasManyTasks: activeStep?.[1]?.taskKeys?.length > 1
stepHasBranch: !!checkCanStepBranch(activeStep, workflow?.tasks)
}

if (!workflow) return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,10 @@ function EditStepDialog({
<div className="dialog-footer flex-row">
<button
className="big flex-item"
disabled={!!enforceLimitedBranchingRule?.stepHasBranch}
onClick={handleClickAddTaskButton}
type="button"
>
Add New Task
Add another Task to this Page
</button>
<button
className="big done"
Expand All @@ -121,9 +120,7 @@ EditStepDialog.propTypes = {
allTasks: PropTypes.object,
deleteTask: PropTypes.func,
enforceLimitedBranchingRule: PropTypes.shape({
stepHasBranch: PropTypes.bool,
stepHasOneTask: PropTypes.bool,
stepHasManyTasks: PropTypes.bool
stepHasBranch: PropTypes.bool
}),
onClose: PropTypes.func,
step: PropTypes.object,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import DrawingTask from './types/DrawingTask.jsx';
import QuestionTask from './types/QuestionTask.jsx';
import TextTask from './types/TextTask.jsx';
import UnknownTask from './types/UnknownTask.jsx';
import DeleteIcon from '../../../../icons/DeleteIcon.jsx';

const taskTypes = {
'drawing': DrawingTask,
Expand All @@ -21,7 +22,28 @@ function EditTaskForm({ // It's not actually a form, but a fieldset that's part
taskIndexInStep,
updateTask
}) {
if (!task || !taskKey) return <li>ERROR: could not render Task</li>;
if (!task || !taskKey) return (
<fieldset
className="edit-task-form"
>
<div className="flex-row">
<span className="task-key">{taskKey || '???'}</span>
<p className="flex-item">
ERROR: could not render Task
{!task && ' (it doesn\'t exist in workflow.tasks)'}
{task?.type && ` of type: ${task.type}`}
</p>
<button
aria-label={`Delete Task ${taskKey || '???'}`}
className="big"
onClick={() => { deleteTask(taskKey) }}
type="button"
>
<DeleteIcon />
</button>
</div>
</fieldset>
);

const TaskForm = taskTypes[task.type] || UnknownTask;

Expand All @@ -48,9 +70,7 @@ function EditTaskForm({ // It's not actually a form, but a fieldset that's part
EditTaskForm.propTypes = {
deleteTask: PropTypes.func,
enforceLimitedBranchingRule: PropTypes.shape({
stepHasBranch: PropTypes.bool,
stepHasOneTask: PropTypes.bool,
stepHasManyTasks: PropTypes.bool
stepHasBranch: PropTypes.bool
}),
stepHasManyTasks: PropTypes.bool,
task: PropTypes.object,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ function QuestionTask({
id={`task-${taskKey}-multiple`}
type="checkbox"
checked={isMultiple}
disabled={enforceLimitedBranchingRule?.stepHasManyTasks && isMultiple /* If rule is enforced, you can't switch a Multi Question Task to a Single Question Task. */}
disabled={enforceLimitedBranchingRule?.stepHasBranch && isMultiple /* If rule is enforced, you can't switch a Multi Question Task to a Single Question Task. */}
onChange={(e) => {
setIsMultiple(!!e?.target?.checked);
}}
Expand Down Expand Up @@ -197,9 +197,7 @@ function QuestionTask({
QuestionTask.propTypes = {
deleteTask: PropTypes.func,
enforceLimitedBranchingRule: PropTypes.shape({
stepHasBranch: PropTypes.bool,
stepHasOneTask: PropTypes.bool,
stepHasManyTasks: PropTypes.bool
stepHasBranch: PropTypes.bool
}),
stepHasManyTasks: PropTypes.bool,
task: PropTypes.object,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function NewTaskDialog({

// The Question Task is either a Single Answer Question Task, or a Multiple Answer Question Task.
// By default, this is 'single', but under certain conditions, a new Question Task will be created as a Multiple Answer Question Task.
const questionTaskType = (!enforceLimitedBranchingRule?.stepHasOneTask) ? 'single' : 'multiple'
const questionTaskType = (!enforceLimitedBranchingRule?.stepHasBranch) ? 'single' : 'multiple'

return (
<dialog
Expand Down Expand Up @@ -125,9 +125,7 @@ function NewTaskDialog({
NewTaskDialog.propTypes = {
addTask: PropTypes.func,
enforceLimitedBranchingRule: PropTypes.shape({
stepHasBranch: PropTypes.bool,
stepHasOneTask: PropTypes.bool,
stepHasManyTasks: PropTypes.bool
stepHasBranch: PropTypes.bool
}),
openEditStepDialog: PropTypes.func,
stepIndex: PropTypes.number
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ const DEFAULT_HANDLER = () => {};

export default function BranchingNextControls({
allSteps = [],
stepKey,
task,
taskKey,
updateNextStepForTaskAnswer = DEFAULT_HANDLER
}) {
if (!task || !taskKey) return null;

const answers = task.answers || []
const answers = task.answers || [];
const selectableSteps = allSteps.filter(([otherStepKey]) => otherStepKey !== stepKey);

function onChange(e) {
const next = e.target?.value;
Expand All @@ -36,7 +38,7 @@ export default function BranchingNextControls({
>
Submit
</option>
{allSteps.map(([stepKey, stepBody]) => {
{selectableSteps.map(([stepKey, stepBody]) => {
const taskKeys = stepBody?.taskKeys?.toString() || '(none)';
return (
<option
Expand All @@ -56,6 +58,7 @@ export default function BranchingNextControls({

BranchingNextControls.propTypes = {
allSteps: PropTypes.arrayOf(PropTypes.array),
stepKey: PropTypes.string,
task: PropTypes.object,
taskKey: PropTypes.string,
updateNextStepForTaskAnswer: PropTypes.func
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ export default function NextStepArrow({
arrowhead = false,
className = 'icon',
color = 'currentColor',
height = 48,
height = 32,
pad = 0,
strokeWidth = 2,
width = 16
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export default function SimpleNextControls({
const isLinearItem = isLinearWorkflow && !isLastItem;
const showFakeSubmit = isLinearWorkflow && isLastItem;
const showNextPageDropdown = !isLinearWorkflow && !showFakeSubmit;
const selectableSteps = allSteps.filter(([otherStepKey]) => otherStepKey !== stepKey);

function onChange(e) {
const next = e.target?.value;
Expand All @@ -38,7 +39,7 @@ export default function SimpleNextControls({
>
Submit
</option>
{allSteps.map(([otherStepKey, otherStepBody]) => {
{selectableSteps.map(([otherStepKey, otherStepBody]) => {
const taskKeys = otherStepBody?.taskKeys?.toString() || '(none)';
return (
<option
Expand Down
Loading

0 comments on commit b19edc1

Please sign in to comment.