[MPDX-7359] Complete coaching sidebar #816
Conversation
| }, | ||
| { | ||
| title: t('Tools'), | ||
| items: ToolsList.flatMap((toolsGroup) => [ |
There was a problem hiding this comment.
This is an unrelated simplification I came across.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| electronicDate: string | null | undefined, | ||
| physicalDate: string | null | undefined, | ||
| ): string | null => { | ||
| if (electronicDate && physicalDate) { |
There was a problem hiding this comment.
I wonder if you could simplidy this by writing. I haven't tested this. just a thought.
const lastNewsletter = new Date(Math.max([electronicDate.getTime(), physicalDate.getTime()]));
There was a problem hiding this comment.
I thought about that. But I liked how this helper function has a single concern: finding the latest date. If I used Math.max, that helper would be either be responsible for finding the latest date and parsing the string into a luxon Date or the helper would need to parse the date for the comparison and then the UI code would need to parse it again to display. Too bad Math.max doesn't work on strings, or I would definitely use it. lodash's max works on strings, but I know in the past you've discouraged using lodash methods. I'm going to leave this the way it is if that's OK.
src/components/Coaching/CoachingDetail/CollapsiblePhoneList.tsx
Outdated
Show resolved
Hide resolved
| export const CollapsibleEmailList: React.FC<CollapsibleEmailListProps> = ({ | ||
| emails, | ||
| }) => { | ||
| const primaryEmail = emails.find((email) => email.primary) ?? emails[0]; |
There was a problem hiding this comment.
For readability it might be worth changing this variable to PrimaryOrFirstEmail but I'm not too fussed.
There was a problem hiding this comment.
Hmm, I meant primary here as "the first email shown to the user" as opposed to the secondary emails that are hidden until they click on the expand icon. I can't quickly think of a different name (firstEmail seems just as inaccurate), so I lean towards leaving this as is.
dr-bizz
left a comment
dr-bizz
left a comment
There was a problem hiding this comment.
Looks great! Can you rename AccountListType to AccountListTypeEnum
| import { CollapsiblePhoneList } from './CollapsiblePhoneList'; | ||
| import { getLastNewsletter } from './helpers'; | ||
|
|
||
| export enum AccountListType { |
There was a problem hiding this comment.
Can you rename this to AccountListTypeEnum as it makes it easier to understand what it is when imported into other files.
Description
Completed the coaching sidebar.
https://jira.cru.org/browse/MPDX-7359
Checklist: