Add fab for announcements #109
Conversation
src/assets/NotificationService.tsx
Outdated
| ] | ||
| } | ||
|
|
||
| export const fetchMarkdownData = async () => { |
There was a problem hiding this comment.
This code looks familiar to somethign I wrote so I'm just wondering - are we duplicating it vs referencing it or did we just move it here?
There was a problem hiding this comment.
Basically, if we can externalize it to one function, or, at least, externalize the constants so they aren't defined 2x, that'd be best. Of course, I haven't verified that this wasn't just moved and everything references this now. So, just a note.
There was a problem hiding this comment.
Or that this is unique. I see it's really just some setters, so might be appropriate. And may even be the only place these constants are now
There was a problem hiding this comment.
OK, I see now that both CommunicationFab and Notifications use the release version and date so they both duplicate the same code essentially and have their own states for it. Instead, we should have that done in one location and imported into the two componenents. That way, they are synchronized, simplified, shared code, and it improves performance. I just intended a quick scan since Anusha is reviewing so feel free to correct me if I'm wrong.
There was a problem hiding this comment.
I agree with Dan's comments, if we can call this code one time instead of duplicate that would be optimal. May be having a server component, make a call there and pass on the data to Communication and Notifications components so you avoid the multiple fetch calls. Just an idea!
|
|
||
| export default function NotificationHistory() { | ||
| const [notifications, setNotifications] = useState<Announcement[]>([]) // TypeScript type for notifications | ||
| const [loading, setLoading] = useState<boolean>(true) // TypeScript type for loading state |
There was a problem hiding this comment.
The notifications comment makes sense, although maybe not needed, as Announcement looks like a custom type. However, the loading and error state comments should be removed because the types are generic types so I'm not sure what the comments are trying to imply?
src/assets/NotificationService.tsx
Outdated
| } | ||
|
|
||
| export const fetchMarkdownData = async () => { | ||
| const releaseVersionURL = 'https://raw.githubusercontent.com/onc-healthit/site-content/master/site-ui-4/version.md' |
There was a problem hiding this comment.
These can be moved to env variables and defined in .env file as there can used during build time. So, you dont have define them twice and just reference the variables.
Includes