Pages Editor: misc updates for internal review

[shaunanoordin]

PR Overview

Part of: Pages Editor MVP project and FEM Lab super-project
Follows #7102
Staging branch URL: https://pr-7105.pfe-preview.zooniverse.org/lab/1982/workflows/editor/3711?env=staging

This PR adds a number of misc updates in preparation for the internal team review, at the end of May. Here's the adapted ToDo List from Slack, 13 May:

Functional:

• Traditional PFE workflows shouldn't be openable in Pages Editor.
  • Current implementation: a warning screen appears to block users from viewing/editing traditional PFE workflows.
  • Future PR thoughts: the warning screen should have an option to perform a one-way conversion from classic PFE workflow to FEM workflow.
  • Note: a "traditional PFE workflow" is a WF with tasks, but no steps.
• Multi-Image Options should be fixed.
  • Fixed: due to a misunderstanding on my part, I conflated the "Flipbook / Separate Frames Viewer" dropdown (which affects workflow.configuration.multi_image_mode) with the "Allow Separate Frames View" checkbox (which affects workflow.configuration.enable_switching_flipbook_and_separate).
• The Limited Branching Rule needs to be updated. See below for more details.
• A Page should not be able to set itself as its Next Page.

Design:

• branching task "next page" arrows should appear outside grey box.
  • NOTE: may clash with the new Limited Branching Rule update. 🤔
• font sizes should be standardised to 16px (16pt?)
  • This is most important for the Simple & Branching Next Step controls

Limited Branching Rule

The Pages Editor implements some incredibly ad-hoc rules on whether a Page can have more than one Question Task.

This is due to some unfortunate "logic overloading", i.e. a Single Answer Question Task (aka type=single) is ALWAYS a Branching Task. This can be a problem if e.g. you want to have 4x Single-Type Tasks on a page, but don't want the page to branch. 🤷

(Also, a smaller reason is that having a Single Answer Question Task can make the design look wonky when it's not the last item on the page, since the yellow "next page" indicators are supposed to sit outside the grey box representing the page. But at least this is something we can figure out later.)

ANYWAY, here's the intended change to the Limited Branching Rule:

OLD:

// 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. 

NEW:

// 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 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. 

NOTE: it's easy to completely disable the Limited Branching Rule by setting this in the TasksPage:

const enforceLimitedBranchingRule = {
  stepHasBranch: false,
  stepHasOneTask: false,
  stepHasManyTasks: false
}

// Super useful if our tests users think the limited branching rule's too confusing.

Status

WIP

Update (2024.05.31 23:30 BST): Ready for review!

[shaunanoordin]

PR Update

This PR is now ready for review!

Here are some additional notes, notably for the "branching task "next page" arrows should appear outside grey box" item

• The experimentalRestyleContainer() function triggers on render.
• This function manually sets a height and margin-bottom for the "step body container", IF the step's last task is a branch task, to make the yellow "next step" arrows jut outside the visible grey container. (see screenshot above)
• WARNING: the container height gets wonky if the window is resized, as the container doesn't resize along with the window.

Additional refinements:

• "Next step" arrows fro branching tasks now look pretty decent if the branching task has 0 answers, or too many answers.

Status

Ready for review! Once this is merged, the Pages Editor will be ready for internal review.

[goplayoutside3]

Looking good! The majority of changes in this PR we've discussed through weekly meetings, so I just double checked that the various features work on this PR branch.

A major styling behavior I didn't notice until now is that the dropdowns appear differently in Safari. There's probably some css rules used in the Pages Editor that are handled differently in various browser. Here's screenshots of Safari:

[goplayoutside3]

Should these lines be permanently removed?

[shaunanoordin]

I was thinking of keeping this in as a warning, since other functions usually have this if (!thing) return safety pattern, but for this function specifically, that safety breaks the code. Maybe I'll rewrite this to a simpler warning note so it doesn't look like commented-out code

[shaunanoordin]

deleteTask() has now been updated:

  function deleteTask(taskKey) {
    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.
    ...

I've reintroduced the safety check for 'workflow', which I shouldn't have removed. Comment added to explain, yo, pls don't check for Task before trying to delete said Task.taskKey === '' is also valid, so the safety doesn't check for if (!taskKey) return either. 👌

[goplayoutside3]

Is this a scenario that could still happen? ("what happens when a Step has multiple branching Questions")

[shaunanoordin]

Good point, we already know the answer to this - this comment should be rewritten as: NOTE: if a step has multiple single-type tasks, only the first is a valid branching task that decides the branching logic of the FEM classifier.

[goplayoutside3]

When I resized my window, it looks like step containers resize as expected 🤔 Do you have screenshots of buggy behavior during resize?

[shaunanoordin]

Sure do:

Screen.Recording.2024-06-07.at.17.32.09.mov

Basically, the issue happens whenever the window resize is sufficient to trigger the row of answers to wrap-around.

[shaunanoordin]

Oh goodness, those Safari visuals! 😭

[coveralls]

coverage: 56.991% (-0.003%) from 56.994%
when pulling bfe191d on pages-editor-pt25
into a698f8c on master.

[coveralls]

coverage: 56.994%. remained the same
when pulling 2278daa on pages-editor-pt25
into a698f8c on master.

[shaunanoordin]

Thanks for the review, Delilah!

Update:

• [design] Safari dropdown visuals have been fixed to be more in line with Chrome and FF!
• Tasks Page's deleteTask()'s safety check has been reinstated to check for workflow, with a note to remind devs not to check for a task before trying to delete said task.
• comments on checkCanStepBranch() updated. (I probably should follow up on that TODO to find out what happens when a single-type task has 0 answers. 🤔 )

Alright, let's go!