[To Main] Implement UX designs for engagement wizard #2573
Conversation
NatSquared
requested review from
Baelx and
jareth-whitney
and removed request for
Baelx
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2573 +/- ##
=======================================
Coverage 76.03% 76.04%
=======================================
Files 609 609
Lines 22076 22080 +4
Branches 1783 1785 +2
=======================================
+ Hits 16786 16790 +4
Misses 5026 5026
Partials 264 264
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thanks for the thorough work as always Nat :)
Could I request that we review the screen reader and keyboard experience for the new wizard? In particular, sighted users get a lot of directions for each field, do nonsighted users get the same? We should use a label tag wherever possible with form fields.
| - Logging functions are left in that aren't required or needed later (with exceptions for error logging, i.e. console.error, etc.) | ||
| - Commented-code is left in that isn't needed later | ||
| - Unsafe function calls like `eval()` | ||
| - Widespread incorrect syntax use (using snake case for many variables in JS when it's not required) | ||
| - Significant amount of repeition in code that can be condensed without much issue (code isn't DRY) * | ||
| - Unnecessary function calls, overly verbose code * | ||
| - Significant amount of repeition in code that can be condensed without much issue (code isn't DRY) \* |
| import { colors } from 'styles/Theme'; | ||
| import { BodyText, Header2 } from '../Typography'; | ||
|
|
||
| export const circleNumberIcons = [ |
There was a problem hiding this comment.
Does const this need to be exported?
| @@ -44,109 +47,126 @@ const EngagementVisibilityControl = () => { | |||
| }); | |||
| return () => subscription.unsubscribe(); | |||
| }, [watch, hasBeenEdited]); | |||
|
|
|||
| useEffect(() => { | |||
| // When the user selects a visibility option and the name has not been edited, set isEditing to true immediately | |||
There was a problem hiding this comment.
It's nice that you built in isEditing from the get-go - given we'll likely re-use this form for editing purposes.
Just for my own understanding, why does visibility selection + untouched engagement name = isEditing
| </Grid> | ||
| <Grid item container xs justifyContent="flex-start" alignItems="flex-start" pb="16px"> | ||
| <Grid item xs={12}> | ||
| <Header2 sx={{ mt: 0, fontSize: '20px', fontWeight: '300' }}>{question}</Header2> |
There was a problem hiding this comment.
It's nice that you've built this handy modular component for the "step" style fields! Can we modify this slightly so that nonsighted users get all the same nice instructions as sighted users? There are multiple ways this could be done but we should definitely use a label tag somewhere and make sure it's tied to the right form field.
This WCAG resource states we should ideally have a label tag and possibly an aria-label to concatenate lengthy instructions. https://www.w3.org/WAI/WCAG21/Understanding/labels-or-instructions.html
| setIsConfirmed(false); | ||
| field.onChange({ target: { value: e.target.value === 'true' } }); | ||
| }} | ||
| aria-label="is_internal" |
There was a problem hiding this comment.
Following up on my previous comment about form labels and sighted vs. nonsighted users, a screen reader is only going to read "is_internal" whereas a sighted user will get the two sentences of instruction on how to fill out this field.
| <Box width="100%"> | ||
| <RadioGroup | ||
| onChange={(e) => setIsSingleLanguage(e.target.value === 'true')} | ||
| aria-label="Select Engagement's Language Type" |
There was a problem hiding this comment.
Thanks for continuing to add aria-labels !
| buttonLabel="Add Language" | ||
| loading={false} | ||
| selectedOptions={selectedLanguages} | ||
| selectLabel="Add Languages" |
| getOptionRequired, | ||
| value, | ||
| isOptionEqualToValue = (option, value) => option === value, | ||
| selectLabel = 'Add Options', |
There was a problem hiding this comment.
Should we maybe remove the default value for these label parameters? If we make them required, it'll force developers to add meaningful labels whenever using this component.
| <SystemMessage status="info"> | ||
| You will be able to modify the configuration of your engagement later in the case the parameters of | ||
| your engagement change. If you prefer, you can use{' '} | ||
| <Link size="small" to="../form"> |
There was a problem hiding this comment.
Always nice that you provide our users with convenience features like this :) links to old versions they may be used to, helpful under constructions messages, etc.
| <Controller | ||
| control={control} | ||
| name="name" | ||
| rules={{ required: 'Engagement title is required' }} |
There was a problem hiding this comment.
Do screen readers read this out if it appears?
There was a problem hiding this comment.
It gets wrapped in the <label> element so it should be!
|
NatSquared
deleted the
feature/DESENG-663-engagement-authoring-wizard-design-compliance
branch
Issue #: https://citz-gdx.atlassian.net/browse/DESENG-663
Description of changes:
User Guide update ticket (if applicable):
N/A