-
Notifications
You must be signed in to change notification settings - Fork 6
Feature 2400 launch review period #2403
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
| import { | ||
| Alert, | ||
| Button, | ||
| Card, |
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.
New MUI components are needed for a new confirmation dialog.
|
|
||
| const TeamReviews = ({ onBack, periodId }) => { | ||
| const { state, dispatch } = useContext(AppContext); | ||
| const history = useHistory(); |
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.
We don't need to use the History API to go back. We just need to call the onBack function that is passed in.
| data: { | ||
| ...period, | ||
| reviewStatus: ReviewStatus.AWAITING_APPROVAL | ||
| reviewStatus: ReviewStatus.OPEN |
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.
When a review period is "launched", its status changes to OPEN, not AWAITING_APPROVAL.
| return null; | ||
| }; | ||
|
|
||
| const requestApproval = async () => { |
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 now appears after the launchReviewPeriod function.
| setValidationMessage(msg); | ||
| if (msg) return; | ||
|
|
||
| const visibleIds = new Set(visibleTeamMembers().map(m => m.id)); |
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 are no validation errors, we now display a modal dialog whose text depends on the number of unapproved review assignments.
|
|
||
| const onPeriodSelected = useCallback( | ||
| period => { | ||
| setSelectedPeriod(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.
Setting this causes the UI to display the TeamReviews page.
| @@ -1,18 +1,17 @@ | |||
| import React from 'react'; | |||
| import renderer from 'react-test-renderer'; | |||
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.
Simplified this snapshot test.
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.
See changes to this file as well by @jackkeller on #2402
| setScore={() => {}} | ||
| /> | ||
| ); | ||
| const input = screen.getByTestId('comment-input').querySelector('input'); |
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.
The input was changed to a textarea.
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.
![]()
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.
I think there may be a conflict following rebase but should be a straight-forward merge once the snapshots are regenerated. Code looks good.
No description provided.