add mission table #2884
add mission table #2884
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2884 +/- ##
========================================
- Coverage 5.93% 5.92% -0.02%
========================================
Files 171 173 +2
Lines 4329 4356 +27
Branches 474 486 +12
========================================
+ Hits 257 258 +1
- Misses 4070 4096 +26
Partials 2 2 ☔ View full report in Codecov by Sentry. |
|
@jracusin I added those links, please try it locally as well and let me know if there is anything you want me to change. From your branch, you should be able to just run |
…nasa.gov into mission-pages-organization
|
|
| @@ -165,6 +174,10 @@ export function useRecaptchaSiteKey() { | |||
| return useLoaderDataRoot()?.recaptchaSiteKey | |||
| } | |||
|
|
|||
| export function WithCredentials() { | |||
There was a problem hiding this comment.
React hooks should have names that start with use:
| export function WithCredentials() { | |
| export function useCredentials() { |
| let publicConsumerCredential | ||
| if (user) { | ||
| const machine = await ClientCredentialVendingMachine.create(request) | ||
| publicConsumerCredential = (await machine.getClientCredentials())?.find( | ||
| (x) => x.scope == 'gcn.nasa.gov/kafka-public-consumer' | ||
| )?.client_id | ||
| } |
There was a problem hiding this comment.
Oof. This will result in Cognito API calls for loading any page at all. I don't think that this is acceptable from a performance perspective.
| destination: 'quickstart' | 'email' | ||
| otherAlerts?: string[] // Alerts specifically under the 'Other' tab, since they can't be generically selected as all | ||
| }) { | ||
| const userCredentials = WithCredentials() |
There was a problem hiding this comment.
Please don't pre-select a client ID for the user.
There was a problem hiding this comment.
This is necessary, at least if the point of this component is to be a shortcut to the prefilled alerts page. Otherwise this may as well just be a link to the streaming quickstart
| ? otherAlerts?.map((alert) => `&alerts=${alert}`).join('') | ||
| : (format == 'text' ? NoticeTypes[alertKey] : JsonNoticeTypes[alertKey]) | ||
| ?.map((alert) => `&alerts=${alert}`) | ||
| .join('') |
There was a problem hiding this comment.
Please construct the URL search params using URLSearchParams rather than with string formatting, so that there is not question of whether we have correctly escaped everything.
| @@ -0,0 +1,134 @@ | |||
| export const NoticeTypes = { | |||
There was a problem hiding this comment.
This appears to be copy-pasted from NoticeTypeCheckboxes. It is fine to factor this data into a separate file, but please remove one copy or the other.
| export default function SelectedAlertShortcutLink({ | ||
| alertKey, | ||
| format, | ||
| otherAlerts, | ||
| destination, | ||
| }: { | ||
| alertKey: string | ||
| format: 'json' | 'text' | ||
| destination: 'quickstart' | 'email' | ||
| otherAlerts?: string[] // Alerts specifically under the 'Other' tab, since they can't be generically selected as all | ||
| }) { |
There was a problem hiding this comment.
There is actually not much code shared in common among the various if statement branches. Would this be cleaner as several different components?
Lots of if statements are often a hint that inheritance or function composition may be a beneficial refactoring.
| useFeature('SVOM') && ( | ||
| <NavLink key="svom" to="svom"> | ||
| SVOM | ||
| </NavLink> | ||
| ), | ||
| <> | ||
| <NavLink key="archival" to="archival"> | ||
| Archival | ||
| </NavLink> | ||
| <SideNavSub | ||
| base="archival" | ||
| items={[ | ||
| <NavLink key="agile" to="agile"> | ||
| AGILE | ||
| </NavLink>, | ||
| <NavLink key="burstcube" to="burstcube"> | ||
| BurstCube | ||
| </NavLink>, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
For the current page highlighting to work, you'll need to move the archival missions' URL paths from, e.g., /missions/burstcube to /missions/archival/burstcube. Please do that, and add /missions/burstcube as a permanent redirect to /missions/archival/burstcube.
Since this move is pretty cut-and-dry but the shortcut links are not (see above), I suggest doing the move and adding the archival mission submenu in its own separate PR.
There was a problem hiding this comment.
This is separated out into PR #2913.
jracusin
changed the title
Description
Adds mission table summarizing all Notice producer missions including their mission status, GCN classic archive or if they're available in GCN Kafka. It also moves archival missions to an archival side menu sub page.
Related Issue(s)
Resolves #2849 and #2850