Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pages Editor: add "Add New Task" Dialog #6915

Merged
merged 21 commits into from
Nov 23, 2023
Merged

Pages Editor: add "Add New Task" Dialog #6915

merged 21 commits into from
Nov 23, 2023

Conversation

shaunanoordin
Copy link
Member

@shaunanoordin shaunanoordin commented Nov 3, 2023 •
edited
Loading

PR Overview

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

This PR adds the "Add New Task" Dialog.

Expected behaviour:

  • On the Tasks Page:
    • clicking "Add a new Task" button should open the Add New Task dialog
  • On the Add New Task Dialog:
    • clicking on a Task type should...
      • create a new Step, e.g. P0
      • create a new Task, corresponding to the Task type, e.g. T0.
      • add the new Task to the new Step.
      • update the workflow resource with the task and steps (NOTE: most of the above has a POC with experimentalAddNewTaskWithStep())
      • close the Add New Task dialog, then open the Edit Step dialog with the new Step selected.
    • pressing Esc or clicking the 'x' close button should close the dialog
  • On the Edit Step dialog:
    • pressing Esc or clicking the 'x' close button should close the dialog
    • No other actions for now, to be resolved in a follow up PR.

Design: Tasks Page, with 0 Tasks

image

Design: "Add a new Task" dialog

image

Design: "Create Task" dialog (actually, it's the "Edit Step" dialog)

image

Code notes:

Status

WIP

@coveralls
Copy link

coveralls commented Nov 3, 2023 •
edited
Loading

Coverage Status

coverage: 57.053%. remained the same
when pulling 7ace391 on pages-editor-pt8
into 8139dbd on master.

eatyourgreens
eatyourgreens reviewed Nov 3, 2023
View reviewed changes
app/pages/lab-pages-editor/components/TasksPage/components/NewTaskButtonAndDialog.jsx Outdated
Add a new Task +
</button>
<dialog
autofocus="true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
autofocus="true"
autoFocus

This is why you're getting ESLint warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't focus the dialog itself by the way.

Focus the first focusable element or the first heading.

The WAI-ARIA dialog example has some discussion of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this dialog supposed to be modal, by the way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore me, I just saw that you're using showModal(). 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The autofocus thing here is an odd one - I think I'll just delete it altogether since adding/removing autofocus="true" has no effect on the UI/UX (on macOS + Chrome 119/FF 119/Safari 15). i.e. the first interactable element (i.e. the close button) always gets focus on showModal() anyway. 🤨

Other misc dev notes:

  • autofocus="true" in React renders as <dialog autofocus="true">...</dialog> in the HTML
  • autofocus, autofocus={true}, autoFocus, autoFocus={true}, and autoFocus="true" will all just render as <dialog>...</dialog>
  • I wondered if this is just a quirk with me adding autofocus to <dialog> instead of a proper interactable element, so I tried... 
    <dialog>
  <input type="text" defaultValue="this is just a distraction" />
  <input type="text" defaultValue="this should get focus when modal opens" autofocus="true" />
</dialog>
  • ...and the ONLY time that the second text input gets focus on .showModal() is with autofocus="true". autoFocus doesn't work, despite React saying it should.
  • Maybe this is a quirk with React 17? I don't know, but I'm making a note of it just in case.

@shaunanoordin
Copy link
Member Author

PR Update

Change of plans for this PR:

  • After a Task type is chosen in the Add New Task dialog, the Edit Step dialog no longer appears.
    • Previous intended behaviour for this PR: clicking on a Task type should add the Task in a Step, then close the Add New Task dialog, then open the Edit Step dialog with the new Step selected.
    • Now: clicking on a Task type should add the Task in a Step, then close the Add New Task dialog, and that's it.
  • This intended behaviour will be re-visited in a follow up PR.

Other than that, all intended behaviour has been implemented in this PR.

Testing

Video: example of intended behaviour. (macOS + Chrome)

Screen.Recording.2023-11-10.at.22.01.40.mov

Status

Ready for review!

@shaunanoordin shaunanoordin requested a review from a team November 10, 2023 22:07
@shaunanoordin shaunanoordin marked this pull request as ready for review November 10, 2023 22:16
@shaunanoordin shaunanoordin force-pushed the pages-editor-pt8 branch 2 times, most recently from 583f7ff to 06f386a Compare November 13, 2023 13:58
@shaunanoordin shaunanoordin mentioned this pull request Nov 13, 2023
Merged
eatyourgreens
eatyourgreens suggested changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but the FontAwesome icons need to either add support for alt text or remove role='img'.

app/pages/lab-pages-editor/components/TasksPage/components/NewTaskButtonAndDialog.jsx Outdated
onClick={openNewTaskDialog}
type="button"
>
Add a new Task +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic point, but maybe add the '+' with ::after, to give a more natural button label.

app/pages/lab-pages-editor/components/TasksPage/components/NewTaskButtonAndDialog.jsx Outdated
Comment on lines 55 to 62
<button
className="plain"
onClick={closeNewTaskDialog}
type="button"
>
<CloseIcon />
</button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This button has no label. That's a level A accessibility failure.

app/pages/lab-pages-editor/icons/CloseIcon.jsx Outdated

export default function CloseIcon() {
return (
<span className="icon fa fa-close" role="img" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is supposed to be an image, it should allow for alt text to be passed in as an aria-label prop. All images must have alt text. If not, then remove the role='img'.

app/pages/lab-pages-editor/icons/CloseIcon.jsx Outdated
Comment on lines 3 to 7
export default function CloseIcon() {
return (
<span className="icon fa fa-close" role="img" />
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export default function CloseIcon() {
return (
<span className="icon fa fa-close" role="img" />
);
}
export default function CloseIcon({ ariaLabel = '' }) {
return (
<span className="icon fa fa-close" role="img" aria-label={ariaLabel} />
);
}

Something like this might work.

app/pages/lab-pages-editor/icons/CopyIcon.jsx Outdated

export default function CopyIcon() {
return (
<span className="icon fa fa-copy" role="img" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing alt text for this image. Either add alt text or remove role='img'.

app/pages/lab-pages-editor/icons/DeleteIcon.jsx Outdated

export default function DeleteIcon() {
return (
<span className="icon fa fa-trash" role="img" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing alt text for this image. Either add alt text or remove role='img'.

app/pages/lab-pages-editor/icons/EditIcon.jsx Outdated

export default function EditIcon() {
return (
<span className="icon fa fa-pencil" role="img" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing alt text for this image. Either add alt text or remove role='img'.

app/pages/lab-pages-editor/icons/MoveDownIcon.jsx Outdated

export default function MoveDownIcon() {
return (
<span className="icon fa fa-caret-down" role="img" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing alt text for this image. Either add alt text or remove role='img'.

app/pages/lab-pages-editor/icons/MoveUpIcon.jsx Outdated

export default function MoveUpIcon() {
return (
<span className="icon fa fa-caret-up" role="img" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing alt text for this image. Either add alt text or remove role='img'.

kieftrav
kieftrav approved these changes Nov 19, 2023
View reviewed changes
Copy link
Contributor

@kieftrav kieftrav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Strong suggestion to add alt-text to all the images for accessibility benefits.

@shaunanoordin
Copy link
Member Author

Thanks everyone! I've incorporated the feedback into the latest update.

PR Update

  • Icons now support an optional alt/aria-label value
    • Icons such as CloseIcon, EditIcon, etc all accept an optional alt parameter. If specified, the icon adds aria-label and role=img to the rendered element.
    • e.g. <CloseIcon /> => <span class="fa fa-close" />
    • e.g. <CloseIcon alt="close dialog" /> => <span class="fa fa-close" aria-label="close dialog" role="img" />
  • Missing alt/aria-label added:
    • "Close Dialog" (X) button now says exactly that
    • Task Icons in TaskItem now describe the task.
    • Note that Task Icons in the NewTaskButtonAndDialog component pull their descriptions from the surrounding button.
  • Decoration tweak: that + in Add New Task moved to a CSS decorator class.

Let's go!

@shaunanoordin shaunanoordin enabled auto-merge (squash) November 22, 2023 23:05
@shaunanoordin shaunanoordin merged commit a014217 into master Nov 23, 2023
3 checks passed
@shaunanoordin shaunanoordin deleted the pages-editor-pt8 branch November 23, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kieftrav kieftrav kieftrav approved these changes

@eatyourgreens eatyourgreens eatyourgreens approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shaunanoordin @coveralls @eatyourgreens @kieftrav