Feature 2221 review period status #2331
Conversation
| style={{ | ||
| color: guild.community ? 'var(--oci-orange)' : 'var(--oci-light-blue)' | ||
| }} | ||
| // title={`This is a ${guild.community ? 'Community' : 'Guild'}.`} |
There was a problem hiding this comment.
This line was recently removed in another PR in order to get the tests to pass in a repeatable way.
But I think we need to add it back to satisfy a story AC that this icon has a tooltip.
Maybe we can determine why including this line causes the tests to break.
There was a problem hiding this comment.
We could try the MUI tooltip component.
There was a problem hiding this comment.
I did a quick dive into Dana Woodman's react-fontawesome library yesterday and did not see anything in the repo history (e.g. discussions, closed issues) that would indicate this behavior was expected.
Ideally we would determine the root cause and, if related to the upstream lib, prepare a minimal reproduction and float them an issue. Although Michael's suggestion to add a Tooltip component may circumvent the need for that work, saving us time in case it's not an easy issue to repro minimally. But from what I can tell all we would need to repro is a Router/MemoryRouter and a font-awesome icon with the title attribute set.
There was a problem hiding this comment.
@vhscom definitely wasn't suggesting we shouldn't drop an issue somewhere. Just trying to meet the AC without waiting for a fix. 😉
My 💰 is on MUI being at fault for this one.
| @@ -1,10 +1,16 @@ | |||
| import React, { useCallback, useContext, useEffect, useState } from 'react'; | |||
There was a problem hiding this comment.
I combined and reordered some imports.
| import DeleteIcon from '@mui/icons-material/Delete'; | ||
| import UnarchiveIcon from '@mui/icons-material/Unarchive'; | ||
| import WorkIcon from '@mui/icons-material/Work'; | ||
| import { |
There was a problem hiding this comment.
I added icons to correspond to all the possible review period status values.
| selectUserProfile | ||
| } from '../../../context/selectors'; | ||
|
|
||
| import { titleCase } from '../../../helpers/strings.js'; |
There was a problem hiding this comment.
This is used to convert a string status value that is all uppercase to one that uses title case.
| UNKNOWN: 'UNKNOWN' | ||
| }; | ||
|
|
||
| const reviewStatusIconMap = { |
There was a problem hiding this comment.
This maps review status values to icons.
| } | ||
| > | ||
| {reviewStatus === ReviewStatus.OPEN ? ( | ||
| <ArchiveIcon /> |
There was a problem hiding this comment.
Changing the way these icons are imported required changing their names here.
| onClick={() => onPeriodClick(id)} | ||
| > | ||
| <Avatar> | ||
| <WorkIcon /> |
There was a problem hiding this comment.
Instead of displaying the same icon for every review period, we now display an icon that indicates the status of the review period.
| name + | ||
| (reviewStatus === ReviewStatus.OPEN ? ' - Open' : '') | ||
| } | ||
| primary={`${name} - ${titleCase(reviewStatus)}`} |
There was a problem hiding this comment.
We now display the actual status string instead of only displaying "Open" or nothing.
| @@ -0,0 +1,5 @@ | |||
| export const titleCase = text => | |||
There was a problem hiding this comment.
This is a new utility function.
| expect(titleCase('test')).toBe('Test'); | ||
| expect(titleCase('one two three')).toBe('One Two Three'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Typically for string formatting utils I'd use a name like format.js for the helper. This allows for the file to format more than just strings without needing to be renamed later, or a new file created for other types of formatting.
vhscom
left a comment
vhscom
left a comment
There was a problem hiding this comment.
LGTM. One suggestion on the name of the helper file.
|
Sorry @vhscom, I merged before seeing your comment about the name of the helper file. What I was going for is having a single helper file where all functions related to strings could go. Naming it "format.js" would narrow the scope to just formatting. |
No description provided.