Feature/dep272: Added start/end time to engagement config, other misc fixes. - #2864
Conversation
…l dates to store in UTC and not Pacific. Updated time conversion functions and usage of them. Updated some visual components to reduce flicker and give fresh data. Allowed modifying of engagement config while it's scheduled but not published.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2864 +/- ##
==========================================
- Coverage 75.17% 75.15% -0.02%
==========================================
Files 517 517
Lines 20067 20107 +40
Branches 1720 1726 +6
==========================================
+ Hits 15085 15112 +27
- Misses 4973 4986 +13
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
NatSquared
left a comment
There was a problem hiding this comment.
Looks good; thanks for all the work cleaning up the utcnow() calls. Just a few questions, including a few about replacing the formatDate function which did not include the time with the convertToPacific function which does.
| if engagement.status_id != Status.Draft: | ||
| raise ValueError(f'Engagement {engagement_id} is not in draft status') | ||
| if engagement.status_id not in valid_statuses: | ||
| raise ValueError(f'Engagement {engagement_id} has already been published, cannot update slug') |
There was a problem hiding this comment.
A semicolon works better to join these two related but independent clauses
There was a problem hiding this comment.
I can't argue
| </Grid> | ||
| <Grid> | ||
| <BodyText sx={{ pl: 2 }}>{formatDate(created_date)}</BodyText> | ||
| <BodyText sx={{ pl: 2 }}>{formatToPacific(created_date)}</BodyText> |
There was a problem hiding this comment.
Before this change, this page would have shown only the date, but will now also show the time. Is this intentional?
There was a problem hiding this comment.
Fixed. When I first replaced it, it was 1-to-1, then I changed the default formatting. I will use YYYY-MM-DD format here.
| <b>Comment Date: </b> | ||
| </BodyText> | ||
| <BodyText>{formatDate(row.created_date)}</BodyText> | ||
| <BodyText>{formatToPacific(row.created_date)}</BodyText> |
There was a problem hiding this comment.
Same here; formatDate did not include the full time but formatToPacific does
| <Grid container direction="row"> | ||
| <Grid> | ||
| <Grid container direction="row" sx={{ gap: '2rem', flexWrap: { xs: 'wrap', lg: 'nowrap' } }}> | ||
| <Stack sx={stackStyles}> |
There was a problem hiding this comment.
Why are we replacing the Grid with a Stack here?
There was a problem hiding this comment.
I used stack here because both Grid2 and Stack use flex, but stack defaults to column orientation whereas Grid defaults to horizontal/row orientation. It also makes the code a bit more human-readable, since they know the direction just by reading the component name. If you don't like it I can switch it back though.
| <Grid sx={{ '&.MuiGrid-root': { paddingTop: 0 } }}> | ||
| <Button onClick={() => setIsPickingDate(true)} icon={<FontAwesomeIcon icon={faCalendar} />}> | ||
| {numberOfDays ? 'Change Dates' : 'Select Dates'} | ||
| {`${numberOfDays ? 'Change' : 'Select'} Dates and Times`} |
There was a problem hiding this comment.
We may be able to just say "Times" or "Time Range" rather than "Dates and Times"
There was a problem hiding this comment.
ok I'll change it to Time Range
| - Updated auto breadcrumbs component for increased visual stability (less flickering) | ||
| - Updated authoring section for greater visual stability (less flickering) | ||
| - Moved header items (badge and breadcrumbs) out of AuthoringTemplate component, which is forced to re-render because it's keyed | ||
| - Utilized useDefferedValue in bottom nav component for language list |
There was a problem hiding this comment.
| - Utilized useDefferedValue in bottom nav component for language list | |
| - Utilized useDeferredValue in bottom nav component for language list |
There was a problem hiding this comment.
Good catch! I always mix up the double consonant in this word.
…atting, fixed typos and wording.
|



Issue #: https://citz-gdx.atlassian.net/browse/DESENG-272
Description of changes:
User Guide update ticket (if applicable):
Common component changes: