-
Notifications
You must be signed in to change notification settings - Fork 6
Feature 2221 review period status #2331
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,16 @@ | ||
| import React, { useCallback, useContext, useEffect, useState } from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import React, { useCallback, useContext, useEffect, useState } from 'react'; | ||
|
|
||
| import ArchiveIcon from '@mui/icons-material/Archive'; | ||
| import DeleteIcon from '@mui/icons-material/Delete'; | ||
| import UnarchiveIcon from '@mui/icons-material/Unarchive'; | ||
| import WorkIcon from '@mui/icons-material/Work'; | ||
| import { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added icons to correspond to all the possible review period status values. |
||
| Archive, | ||
| BorderColor, | ||
| Delete, | ||
| DoorFront, | ||
| HourglassTop, | ||
| MeetingRoom, | ||
| QuestionMark, | ||
| Unarchive | ||
| } from '@mui/icons-material'; | ||
|
|
||
| import { | ||
| Avatar, | ||
|
|
@@ -56,6 +62,8 @@ import { | |
| selectUserProfile | ||
| } from '../../../context/selectors'; | ||
|
|
||
| import { titleCase } from '../../../helpers/strings.js'; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is used to convert a string status value that is all uppercase to one that uses title case. |
||
|
|
||
| const propTypes = { | ||
| message: PropTypes.string, | ||
| onSelect: PropTypes.func | ||
|
|
@@ -118,6 +126,14 @@ const ReviewStatus = { | |
| UNKNOWN: 'UNKNOWN' | ||
| }; | ||
|
|
||
| const reviewStatusIconMap = { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This maps review status values to icons. |
||
| [ReviewStatus.PLANNING]: <BorderColor />, | ||
| [ReviewStatus.AWAITING_APPROVAL]: <HourglassTop />, | ||
| [ReviewStatus.OPEN]: <MeetingRoom />, | ||
| [ReviewStatus.CLOSED]: <DoorFront />, | ||
| [ReviewStatus.UNKNOWN]: <QuestionMark /> | ||
| }; | ||
|
|
||
| const ReviewPeriods = ({ onPeriodSelected, mode }) => { | ||
| const { state, dispatch } = useContext(AppContext); | ||
|
|
||
|
|
@@ -433,9 +449,9 @@ const ReviewPeriods = ({ onPeriodSelected, mode }) => { | |
| } | ||
| > | ||
| {reviewStatus === ReviewStatus.OPEN ? ( | ||
| <ArchiveIcon /> | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing the way these icons are imported required changing their names here. |
||
| <Archive /> | ||
| ) : ( | ||
| <UnarchiveIcon /> | ||
| <Unarchive /> | ||
| )} | ||
| </IconButton> | ||
| </Tooltip> | ||
|
|
@@ -445,7 +461,7 @@ const ReviewPeriods = ({ onPeriodSelected, mode }) => { | |
| edge="end" | ||
| aria-label="Delete" | ||
| > | ||
| <DeleteIcon /> | ||
| <Delete /> | ||
| </IconButton> | ||
| </Tooltip> | ||
| </> | ||
|
|
@@ -457,17 +473,12 @@ const ReviewPeriods = ({ onPeriodSelected, mode }) => { | |
| key={`period-lia-${id}`} | ||
| onClick={() => onPeriodClick(id)} | ||
| > | ||
| <Avatar> | ||
| <WorkIcon /> | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of displaying the same icon for every review period, we now display an icon that indicates the status of the review period. |
||
| </Avatar> | ||
| <Avatar>{reviewStatusIconMap[reviewStatus]}</Avatar> | ||
| </ListItemAvatar> | ||
| <ListItemText | ||
| key={`period-lit-${id}`} | ||
| onClick={() => onPeriodClick(id)} | ||
| primary={ | ||
| name + | ||
| (reviewStatus === ReviewStatus.OPEN ? ' - Open' : '') | ||
| } | ||
| primary={`${name} - ${titleCase(reviewStatus)}`} | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now display the actual status string instead of only displaying "Open" or nothing. |
||
| secondary={getSecondaryLabel(id)} | ||
| /> | ||
| </ListItem> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| export const titleCase = text => | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a new utility function. |
||
| text.replace( | ||
| /\w\S*/g, | ||
| word => word.charAt(0).toUpperCase() + word.substr(1).toLowerCase() | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import { titleCase } from './strings'; | ||
|
|
||
| describe('strings', () => { | ||
| it('can title-case a string', () => { | ||
| expect(titleCase('')).toBe(''); | ||
| expect(titleCase('a')).toEqual('A'); | ||
| expect(titleCase('test')).toBe('Test'); | ||
| expect(titleCase('one two three')).toBe('One Two Three'); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typically for string formatting utils I'd use a name like |
||
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 combined and reordered some imports.