-
Notifications
You must be signed in to change notification settings - Fork 75
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.
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
Pages Editor: design updates for internal test #7129
Conversation
Testing StepsTest with macOS + Chrome or Firefox. If you use Safari, please be warned that the dropdowns look gradient-ly, which is out of design. Testing Feature update: Workflow URLs on the Workflows listing
Testing Feature update: collapsible Help fields
Testing Design updates:
|
<div className="flex-row spacing-bottom-S"> | ||
<label | ||
className="big" | ||
htmlFor={`task-${taskKey}-help`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires the input to be in the DOM at all times. You can’t have a label without its corresponding input in the DOM, so the show/hide pattern used in this PR is broken. It works visually but produces a broken DOM when the text input is removed. That won’t work in a screen reader.
This needs to use a details/summary pattern, or the correct ARIA roles and state for a collapsible panel, in order to work for any users that can't physically see the task editor.
I’d also suggest using the HTML hidden
boolean attribute to hide the input, rather than removing it from the DOM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the WAI-ARIA pattern for building a disclosure widget, including roles, states and keyboard interactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, of course, you could use the native HTML disclosure widget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scott O'Hara summarised the pros and cons of using a native disclosure widget vs. rolling your own in JavaScript. You can use that to make a decision as to which fits best here.
PR Update
|
@shaunanoordin you're welcome. You'll still need to add the expanded state etc. to your toggle button, so that it's announced as open/closed for blind users. I'll add a comment to the PR. |
app/pages/lab-pages-editor/components/TasksPage/components/EditStepDialog/types/DrawingTask.jsx
Outdated
Show resolved
Hide resolved
...pages/lab-pages-editor/components/TasksPage/components/EditStepDialog/types/QuestionTask.jsx
Outdated
Show resolved
Hide resolved
app/pages/lab-pages-editor/components/TasksPage/components/EditStepDialog/types/TextTask.jsx
Outdated
Show resolved
Hide resolved
Interesting UX choices here. Let's see how it tests first, but I hope we can revisit the '1 Help text entry per page' discussion at a later stage to really sort this out. 1 minor styling nitpick: Can we change the Help Text header to be less bold than the Main Text/Text Task header style? |
d8a1927
to
7bcfb06
Compare
Very minor styling point. Since you're hiding the input with |
55c4a95
to
740ed7c
Compare
740ed7c
to
bfc6ff1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected and overall looks good to me (tested on Chrome). One strong recommendation to refactor the shared aspects of the different Tasks so as to de-duplicate code & prevent future issues if someone changes one file without knowledge of the others.
Help Text | ||
</label> | ||
<button | ||
aria-label={`Show/Hide Help field`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can modify this text to be more accurate to the existing state:
${(showHelpField) ? 'Hide' : 'Show'} Help Field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would change the labels to 'Show help field (closed)' and 'Hide help field (expanded)', since the ARIA expanded state is read out as part of the label. I don't think it's recommended practice to change the labels for toggles, based on the toggle state. Rather, have a single toggle button label that makes sense when accompanied by the open/closed state.
The visible label could be changed, though, for sighted users.
> | ||
Help Text | ||
</label> | ||
<div className="flex-row spacing-bottom-S"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd strongly consider refactoring the shared code between DrawingTask / TextTask / QuestionTask into a file called SharedTaskFields
or something like that. Without your context, I could imagine it being easy to change something in one and miss the others.
Co-authored-by: Jim O'Donnell <[email protected]>
Co-authored-by: Jim O'Donnell <[email protected]>
Co-authored-by: Jim O'Donnell <[email protected]>
bfc6ff1
to
1f9d10d
Compare
Thanks Travis!
|
The aria-label shouldn’t change when the button is pressed. That can make the UX confusing in a screen reader. |
PR Overview
Part of: Pages Editor MVP project and FEM Lab super-project
Follows #7105
Staging branch URL: https://pr-7129.pfe-preview.zooniverse.org/lab/1982/workflows/editor/3711?env=staging
This PR adds a number of small design updates, in preparation for additional internal tests. Here's the ToDo List from Slack, 10 Jun:
BranchingNextPage: answers' Next Page should have "New Task" optionWon't be done in this PRselect { -webkit-appearance: none }
to remove a weird coloured gradient overlay on all HTML<select>
dropdown elements. Unfortunately, this also removed the decorative "collapse/expand" carats/arrows on the dropdowns of EVERY browser. I've decided to ignore relatively small style issues on Safari, if the workarounds end up borking more browsers.Status
Ready for review!