[To Main] DESENG-655: Add existing engagement fields to engagement wizard #2555
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2555 +/- ##
==========================================
+ Coverage 75.95% 75.96% +0.01%
==========================================
Files 607 609 +2
Lines 21943 21959 +16
Branches 1707 1711 +4
==========================================
+ Hits 16666 16681 +15
- Misses 5014 5015 +1
Partials 263 263
Flags with carried forward coverage won't be shown. Click here to find out more.
|
NatSquared
changed the title
There was a problem hiding this comment.
Thanks Nat! Lots of great work here. Could I ask for a couple of things:
- ensure textfields and other inputs have labels wherever possible
- remove commented-out code
Also another gentle reminder to just keep an eye on the file count when working on code :) If you're getting to 15-ish files and there's still work left, it could be a good time to submit a PR
| @@ -0,0 +1,237 @@ | |||
| import React, { useEffect } from 'react'; | |||
There was a problem hiding this comment.
Was the existing date calculation not working? Or did the designs include a day calculator?
There was a problem hiding this comment.
The designs included a day calculation - though "DatesPicker" would probably have been a better name for this component
| color: colors.type.inverted.primary, | ||
| borderRadius: '0 50% 50% 0', | ||
| border: `1px solid ${colors.surface.blue[80]}`, | ||
| borderLeft: 'none', |
There was a problem hiding this comment.
A lot of this styling is repeated from above. You could make things more compact by just having a buttonRadius variable set based on the condition fired. Also, is it possible to merge/reduce some of the conditions in this function?
| }; | ||
|
|
||
| return ( | ||
| <Grid container mt={2} direction="column" spacing={2}> |
There was a problem hiding this comment.
Is this component a date picker too? If so, we could make that more explicit by naming it DatePickerWithDateCaclulation or something like that
| <Grid item container direction="row"> | ||
| <Grid item> | ||
| <BodyText bold>Start Date</BodyText> | ||
| <Controller |
There was a problem hiding this comment.
Since we're building this component from scratch again, how does it fare with keyboards only? What about with a screen reader?
There was a problem hiding this comment.
After testing, I believe this is on par with the existing calendar picker - MUI has excellent screen reader info built into the component, and the alternative is to assume that the user's browser has a calendar picker.
| </Grid> | ||
| <Grid item> | ||
| <BodyText thin display="inline"> | ||
| (12:01 AM) |
There was a problem hiding this comment.
What is this static text used for?
There was a problem hiding this comment.
This was part of the design - It clarifies the exact time that engagements will go live
| )} | ||
| /> | ||
| </FormControl> | ||
| <When condition={isInternal !== undefined}> |
There was a problem hiding this comment.
I'm trying to understand this condition, so when isInternal is true or false it'll show the Grid, but when it's undefined it won't? Is undefined the default value it gets and it would be complicated to make it false from the get-go? If so, maybe add a comment to explain
| slug: formData.get('slug') as string, | ||
| }); | ||
| } catch (e) { | ||
| console.error(e); |
There was a problem hiding this comment.
A context message could be helpful in case we need to go and track down this error message
| console.error(e); | |
| console.error('Error updating engagement slug', e); |
| maxLength={50} | ||
| title="1. What is the title of your Engagement?" | ||
| instructions="Titles should succinctly describe what your engagement is about in 60 characters or less." | ||
| placeholder="Engagement title" |
There was a problem hiding this comment.
Many screen readers do not read placeholders and some may not properly interpret the title attribute on a form field. We should make sure to include a label so the field is properly identified by them.
| placeholder="Engagement title" | |
| placeholder="Engagement title" | |
| label="Engagement title" |
| <TextField | ||
| value={currentSlug} | ||
| onChange={setCurrentSlug} | ||
| title="Engagement URL" |
There was a problem hiding this comment.
It seems like maybe there's a precedent in this app for using the title attribute instead of having form labels? WCAG guidelines say we should go for labels whenever possible. Titles are somewhat acceptable but may be unreliable in terms of conveying field info to screen readers: https://www.w3.org/WAI/tutorials/forms/labels/#using-the-title-attribute
| title="Engagement URL" | |
| label="Engagement URL" |
| @@ -148,18 +148,18 @@ const ScheduleModal = ({ reschedule, open, updateModal }: ScheduleModalProps) => | |||
| <Grid data-testid={'desktop-datepicker'} item xs={6}> | |||
| <MetLabel>Date</MetLabel> | |||
| <DesktopDatePicker | |||
| inputFormat="MM/DD/YYYY" | |||
| // inputFormat="MM/DD/YYYY" | |||
There was a problem hiding this comment.
Are these commented-out fields needed for later?
|
| @@ -66,8 +67,9 @@ const EngagementVisibilityControl = () => { | |||
| )} | |||
| /> | |||
| </FormControl> | |||
| <When condition={isInternal !== undefined}> | |||
| <When condition={!isEditing}> | |||
| {/* Hide the URL settings until the user has selected a visibility option */} | |||
NatSquared
deleted the
feature/DESENG-655-add-existing-fields-to-engagement-wizard
branch
Issue #: 🎟️ DESENG-655
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).