-
Notifications
You must be signed in to change notification settings - Fork 6
Feature 2290 confirm review period #2376
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
Conversation
| const handleLaunchDateChange = (val, period) => { | ||
| const newDate = val?.$d; | ||
| const isoDate = newDate.toISOString() ?? null; | ||
| const isoDate = newDate?.toISOString() ?? null; |
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.
If no launch date was specified, the ? here prevents an error from trying to call toISOString.
| } | ||
| }; | ||
|
|
||
| const validateReviewPeriod = period => { |
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 function implements review period validation that must hold in order to "REQUEST APPROVAL".
| ); | ||
| }; | ||
|
|
||
| const approvalButton = () => { |
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 function determines whether to render a button and what text should be on the button.
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.
Without knowing off hand how many ReviewStatus potentials there are could this have been a ternary?
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.
There are currently five ReviewStatus values, so I'm thinking there will be other button labels in the future.
|
|
||
| import { AddCircle, Archive, Delete, Unarchive } from '@mui/icons-material'; | ||
| import { | ||
| Alert, |
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.
Using for validation error messages.
| const handleCloseDateChange = (val, period) => { | ||
| const newDate = val?.$d; | ||
| const isoDate = newDate.toISOString() ?? null; | ||
| const isoDate = newDate?.toISOString() ?? null; |
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.
If no launch date was specified, the ? here prevents an error from trying to call toISOString.
| const handleSelfReviewDateChange = (val, period) => { | ||
| const newDate = val?.$d; | ||
| const isoDate = newDate.toISOString() ?? null; | ||
| const isoDate = newDate?.toISOString() ?? null; |
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.
If no launch date was specified, the ? here prevents an error from trying to call toISOString.
| {approvalButton()} | ||
| </div> | ||
| )} | ||
| {validationMessage && ( |
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.
If there is a validation error message, display it in an Alert.
vhscom
left a comment
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.
LGTM
jackkeller
left a comment
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.
Left a comment, not a dealbreaker.
No description provided.