-
Notifications
You must be signed in to change notification settings - Fork 6
Feature 2289 review assignments #2365
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
| } | ||
| } | ||
| }, [open]); | ||
| }, [open, selectedMembers]); |
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 initializeChecked function called in this useEffect uses the value of selectedMembers.
| @@ -0,0 +1,13 @@ | |||
| .team-reviews { | |||
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 styles the rows of members and their reviewers.
| AddCircle, | ||
| AddComment, | ||
| Archive, | ||
| ArrowBack, |
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 removed the back button functionality that I added earlier.
The consensus is that users will know to use the browser back button.
| import './TeamReviews.css'; | ||
|
|
||
| const propTypes = { | ||
| teamMembers: PropTypes.arrayOf( |
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 was not one of the props being used.
| onBack: PropTypes.func | ||
| }) | ||
| ), | ||
| onBack: PropTypes.func, |
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 is called when users click the "Request Approval" button.
| const history = useHistory(); | ||
| const location = useLocation(); | ||
|
|
||
| const [assignments, setAssignments] = useState([]); |
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 holds all the assignments (member <-> reviewer) for the current review period.
| const [memberSelectorOpen, setMemberSelectorOpen] = useState(false); | ||
| const [newRequestOpen, setNewRequestOpen] = useState(false); | ||
| const [query, setQuery] = useState({}); | ||
| const [reviewerSelectorOpen, setReviewerSelectorOpen] = useState(false); |
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 controls whether a MemberSelectorDialog is open.
|
|
||
| import { AddCircle, Archive, Delete, Unarchive } from '@mui/icons-material'; | ||
| import { | ||
| AddCircle, |
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 removed a lot of code that is no longer used throughout this file.
| const [reviewerSelectorOpen, setReviewerSelectorOpen] = useState(false); | ||
| const [reviews, setReviews] = useState(null); | ||
| const [selectedMember, setSelectedMember] = useState(null); | ||
| const [selectedReviewers, setSelectedReviewers] = useState([]); |
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 is used by the MemberSelectorDialog.
|
|
||
| const csrf = selectCsrfToken(state); | ||
| const currentMembers = selectCurrentMembers(state); | ||
| const memberMap = currentMembers.reduce((map, member) => { |
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 allows quickly finding a member based on their id.
| }, {}); | ||
| const currentUser = selectCurrentUser(state); | ||
| const isAdmin = selectIsAdmin(state); | ||
| const myTeam = selectMyTeam(state); |
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.
Lots more unused code was removed.
| loadTeamMembers(); | ||
| }, [currentMembers]); | ||
|
|
||
| useEffect(() => { |
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 the circled plus icon for a member is clicked, this gets the reviewers that are currently assigned to that member so they can be passed to the MemberSelectorDialog.
| setSelectedReviewers(reviewers); | ||
| }, [selectedMember]); | ||
|
|
||
| const editReviewers = member => { |
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 is called when the circled plus icon for a member is clicked.
| history.goBack(); | ||
| }, [csrf, dispatch, toDelete, handleConfirmClose]); | ||
|
|
||
| const getReviewers = useCallback( |
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 is called by the editReviewers function.
| loadedReviews.current = false; | ||
| loadReviews(); | ||
| }, [loadReviews]); | ||
| const deleteReviewer = async (member, reviewer) => { |
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 is called when the "x" in a reviewer Chip is clicked to review the corresponding reviewer from the member described by that row.
| } | ||
| }; | ||
|
|
||
| const compareStrings = (s1, s2) => (s1 || '').localeCompare(s2 || ''); |
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 is used by the sortMembers function below.
|
|
||
| const compareStrings = (s1, s2) => (s1 || '').localeCompare(s2 || ''); | ||
|
|
||
| const sortMembers = members => |
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 is used to sort the list of reviewers for each member.
| return compare; | ||
| }); | ||
|
|
||
| const updateReviewers = async (member, reviewers) => { |
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 is called when the SAVE button in the MemberSelectorDialog is clicked.
| <ArrowBack /> Back | ||
| </Button> | ||
| </Tooltip> | ||
| const closeReviewerDialog = () => { |
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 is called when the "X" button in the upper-left of the MemberSelectorDialog is clicked.
| }; | ||
|
|
||
| const REVIEWER_LIMIT = 2; | ||
| const renderReviewers = member => { |
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 renders chips for the reviewers of a given member. Currently a maximum of two are displayed. If their are more, text following the chips indicates the number of reviewers that are not shown.
| label="Close Date" | ||
| disabled={!isAdmin} | ||
| /> | ||
| <Button onClick={requestApproval}> |
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 button sends a request to change the review status of the current review period.
|
|
||
| <List dense role="list" sx={{ height: '50%', overflowY: 'scroll' }}> | ||
| {teamMembers.map(member => ( | ||
| <ListItem |
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 renders a row for one member.
| Box, | ||
| Button, | ||
| FormControl, | ||
| IconButton, |
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.
No longer used.
| Select, | ||
| Skeleton, | ||
| TextField, | ||
| Tooltip, |
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.
No longer used.
| onSelect: PropTypes.func | ||
| }; | ||
| const displayName = 'ReviewPeriods'; | ||
| const feedbackTemplateUrl = '/services/feedback/templates'; |
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.
No longer used.
|
|
||
| const onPeriodClick = useCallback( | ||
| id => { | ||
| if (selectReviewPeriod(state, id)?.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.
We are now handling more status values than OPEN.
| name, | ||
| reviewStatus, | ||
| id, | ||
| launchDate, |
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 next three properties are no longer used. Removing them causes several lines to be reformatted by Prettier.
| word => word.charAt(0).toUpperCase() + word.substr(1).toLowerCase() | ||
| ); | ||
| text | ||
| .replace(/_/g, ' ') |
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 modified this to replace underscores with spaces.
| expect(titleCase('a')).toEqual('A'); | ||
| expect(titleCase('test')).toBe('Test'); | ||
| expect(titleCase('one two three')).toBe('One Two Three'); | ||
| expect(titleCase('one_two_three')).toBe('One Two Three'); |
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 added a test for handling underscores.
| ) : ( | ||
| <TeamReviews | ||
| onBack={() => onPeriodSelected(null)} | ||
| onBack={() => setSelectedPeriod(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.
I corrected the function name.
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.
Approved!
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.
Assuming the TODO is for a later pull. Looks good!
No description provided.