Remove reference to run name for clients and use Table #97
Conversation
nafiz1001
changed the title
nafiz1001
changed the title
| title: "", | ||
| dataIndex: "id", | ||
| key: "metric", | ||
| width: "1%", |
There was a problem hiding this comment.
manually assigning css related values at this level is not good frontend dev practice. its good to have the styling centralized at either the each level of interest, (in the index.scss file, global.scss, etc)
There was a problem hiding this comment.
all the components logic was crammed into this file, i strongly suggest refactoring this to be more modular
There was a problem hiding this comment.
i dont think the name is indicative of what is truly happening within the component, there is useDatasetColumns hook, the MetricButton, the StagingButton and Expiration components with each their own respective logic in the file.
There was a problem hiding this comment.
I see. I could create other files like "hook.ts" or "helpers.ts"
There was a problem hiding this comment.
yes for the hooks.ts but no need to have a helper file, you could use the other components folder structure as an example, ie : the StagingButton is a component in itself with its interface and logic.
| currentRequest.status !== "FAILED" | ||
| ) { | ||
| const { type, status } = currentRequest | ||
| const actions: ActionDropdownProps["actions"] = [ |
There was a problem hiding this comment.
this dropdown can be simplified, an example can be found in src\components\ProjectActionsDropdown\index.tsx
There was a problem hiding this comment.
although i am ambivalent towards this change, i dont think it is necessary
using the imported ExternalProjectID type in order to assign string into the already string assigned variable unless the ExternalProjectID has some extra conditions, i dont see the relevance.
There was a problem hiding this comment.
It's mainly in case external project id changes type from string to something else. It indeed has no relevance currently, but imo type is a great way of enforcing the meaning of a variable despite what @UlysseFG would say.
| }, []) | ||
| } | ||
|
|
||
| interface MetricProps { |
There was a problem hiding this comment.
there are multiple interfaces in this file. i suggest to refactor this file to be more modular, as per my previous comment.
| useEffect(() => { | ||
| ;(async () => { | ||
| const datasets = await dispatch(fetchDatasets(externalProjectID)) | ||
| setDataSource( | ||
| datasets.map((dataset) => ({ | ||
| id: dataset.id, | ||
| lane: dataset.lane, | ||
| external_project_id: dataset.external_project_id, | ||
| latest_release_update: new Date( | ||
| dataset.latest_release_update, | ||
| ), | ||
| isFetchingRequest: true, | ||
| activeRequest: undefined, | ||
| totalSize: 0, // size of 0 indicates that the size is not yet fetched | ||
| })), | ||
| ) | ||
| setIsFetching(false) | ||
| const readsets = await dispatch( | ||
| fetchReadsets(datasets.map((dataset) => dataset.id)), | ||
| ) | ||
| const requests = await dispatch( | ||
| fetchRequests(datasets.map((dataset) => dataset.id)), | ||
| ) | ||
| const totalSizeByDatasets = readsets.reduce< | ||
| Record<TritonDataset["id"], number> | ||
| >((acc, readset) => { | ||
| acc[readset.dataset] = | ||
| (acc[readset.dataset] || 0) + readset.total_size | ||
| return acc | ||
| }, {}) | ||
| const activeRequestByDatasets = requests.reduce< | ||
| Record<TritonDataset["id"], TritonRequest | undefined> | ||
| >((acc, request) => { | ||
| acc[request.dataset_id] = request | ||
| return acc | ||
| }, {}) |
There was a problem hiding this comment.
this single useEffect hook function has a lot of logic in it. it would be good to create functions in order to compartmentalize the logic into their own functions (the same approach as with the components but at the function level)
|
this branch needs a rebase and there are some conflicts to resolve |
| onClick={() => { | ||
| if (status === "SUCCESS") { | ||
| Modal.info({ | ||
| title: `Dataset successfully staged`, | ||
| content: [ | ||
| `You can now download the dataset by following the instructions sent to your email. | ||
| If you don't see the email, please check your spam folder. | ||
| If it's still missing, try resetting your password and checking again. | ||
| For further assistance, feel free to contact us at`, | ||
| " ", | ||
| <a | ||
| key={0} | ||
| href={`mailto:${config.supportEmail}`} | ||
| > | ||
| {config.supportEmail} | ||
| </a>, | ||
| ], | ||
| }) | ||
| } | ||
| }} |
There was a problem hiding this comment.
this entire section can be compartmentalized into a more sophisticated component, the newly created component could also handle errors from the received status
| let statusDescription: ReactNode | ||
| if (status === "SUCCESS") { | ||
| statusDescription = "DOWNLOAD" | ||
| } else { | ||
| statusDescription = "QUEUED" | ||
| } |
There was a problem hiding this comment.
this section can be {status === "SUCCESS" ? "DOWNLOAD": "QUEUED"} instead of assigning the statusDescription variable and then using it as a string inside the component
No description provided.