360 - Refactor Dataset Page to handle dataset ID via URL query #364
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There was a problem hiding this comment. Could you give me feedback on this suggestion?
In the future we may consider makeing There was a problem hiding this comment. Thank you for these great suggestions 👍 I totally agree with keeping the prop as |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,9 +19,8 @@ export const ShareDatasetButton = ({ dataset }) => { | |
| }, 3000) | ||
| const [isCopied, setIsCopied] = useState(false) | ||
| const [value, copyText] = useCopyToClipboard(null) | ||
| const id = `shareable-link_${dataset.id}` | ||
| const shareableLink = `${getDomain()}/dataset/${dataset.id}?ref=share` | ||
|
|
||
| const onShareClick = () => { | ||
| openModal(id) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,6 @@ const getExperimentCountBySpecies = (data, experiments) => { | |
| export const DatasetSummary = ({ dataset }) => { | ||
| const { getTotalExperiments, getTotalSamples } = useDatasetManager() | ||
| const { setResponsive } = useResponsive() | ||
| const totalSamples = getTotalSamples(dataset.data) | ||
| const totalExperiments = getTotalExperiments(dataset.data) | ||
| const experimentCountBySpecies = getExperimentCountBySpecies( | ||
|
|
@@ -71,7 +70,7 @@ export const DatasetSummary = ({ dataset }) => { | |
| </TableHeader> | ||
| <TableBody style={{ fontSize: setResponsive('16px', '18px') }}> | ||
| <SpiecesRow | ||
| samplesBySpecies={dataset.organism_samples} | ||
| experimentCountBySpecies={experimentCountBySpecies} | ||
| /> | ||
| <TotalRow totals={[totalSamples, totalExperiments]} /> | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,17 @@ export const DownloadDatasetButton = ({ | |
| dataset, | ||
| label = 'Download Dataset' | ||
| }) => { | ||
| const { | ||
| is_processing: isProcessing, | ||
| is_processed: isProcessed, | ||
| success | ||
| } = dataset | ||
| const { openModal, closeModal } = useModal() | ||
| const id = `download-dataset_${dataset.id}` | ||
| const isUnprocessedDataset = // sets visibility of the Download Dataset button | ||
| !isProcessing && !isProcessed && success !== false | ||
|
|
||
| if (!isUnprocessedDataset) return null | ||
|
|
||
| return ( | ||
| <Modal | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,12 +38,22 @@ const Card = ({ description, format, index, title }) => { | |
| ) | ||
| } | ||
|
|
||
| export const FilesSummary = ({ dataset }) => { | ||
| const { | ||
| organism_samples: organismSamples, | ||
| aggregate_by: aggregateBy, | ||
| scale_by: scaleBy, | ||
| quantile_normalize: quantileNormalize, | ||
| is_processed: isProcessed, | ||
| success | ||
| } = dataset | ||
| const { createDataset, updateDataset, regeneratedDataset } = | ||
| useDatasetManager() | ||
| const { setResponsive } = useResponsive() | ||
| const isDownloadOptions = isProcessed && success // sets visibility of the download options | ||
|
|
||
| // returns the file size estimates of the given dataset and its aggregate_by value (either 'EXPERIMENT' or 'SPECIES') | ||
| const downloadFilesData = (data, samplesBySpecies, aggregateByOption) => { | ||
| const totalExperiments = Object.keys(data).length | ||
| // metadata info of a download https://github.com/AlexsLemonade/refinebio-frontend/issues/25#issuecomment-395870627 | ||
| // the samples aggregated by 'EXPERIMENT' | ||
|
|
@@ -91,16 +101,15 @@ export const FilesSummary = ({ dataset, defaultDataset = {}, isProcessed }) => { | |
| } | ||
| } | ||
|
|
||
| return aggregateByOption === 'SPECIES' | ||
| ? aggregatedBySpecies(dataset, samplesBySpecies) | ||
| : aggregatedByExperiment(dataset) | ||
| } | ||
|
|
||
| const fileSummaries = downloadFilesData( | ||
| dataset.data, | ||
| organismSamples, | ||
| aggregateBy | ||
| ) | ||
| const transformationOptions = options.transformation.reduce( | ||
| (acc, cur) => ({ ...acc, [cur.value]: cur.label }), | ||
|
|
@@ -116,6 +125,7 @@ export const FilesSummary = ({ dataset, defaultDataset = {}, isProcessed }) => { | |
| params | ||
| ) | ||
| const pathname = `/dataset/${response.id}` | ||
| const defaultDataset = {} // TODO: This will be cleaned up in Issue #359 | ||
|
|
||
| gtag.trackRegeneratedDataset( | ||
| defaultDataset, | ||
|
|
@@ -133,17 +143,17 @@ export const FilesSummary = ({ dataset, defaultDataset = {}, isProcessed }) => { | |
| Download Files Summary | ||
| </Heading> | ||
|
|
||
| {isDownloadOptions && ( | ||
| <Box margin={{ bottom: 'small' }}> | ||
| {!openForm && ( | ||
| <Box direction="row" gap="xlarge"> | ||
| <Text weight="bold"> | ||
| Aggregate by: {formatString(aggregateBy)} | ||
| </Text> | ||
| <Text weight="bold"> | ||
| Transformation: {transformationOptions[scaleBy]} | ||
| </Text> | ||
| {!quantileNormalize && ( | ||
| <Text weight="bold"> | ||
| Quantile Normalization Skipped for RNA-seq samples | ||
| </Text> | ||
|
|
@@ -162,7 +172,7 @@ export const FilesSummary = ({ dataset, defaultDataset = {}, isProcessed }) => { | |
| <DownloadOptionsForm | ||
| dataset={dataset} | ||
| buttonLabel="Regenerate Dataset" | ||
| isProcessed={isDownloadOptions} | ||
| onSubmit={handleRegenerateDataset} | ||
| /> | ||
| </ExpandableBlock> | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { useEffect, useState } from 'react' | ||
| import { useRouter } from 'next/router' | ||
| import { Box } from 'grommet' | ||
| import { useDatasetManager } from 'hooks/useDatasetManager' | ||
| import { usePageRendered } from 'hooks/usePageRendered' | ||
|
|
@@ -25,48 +26,48 @@ export const getServerSideProps = ({ query }) => { | |
| return { props: { query } } | ||
| } | ||
|
|
||
| export const Dataset = ({ query: { dataset_id: datasetId, start } }) => { | ||
| const pageRendered = usePageRendered() | ||
| const { push } = useRouter() | ||
| const { setResponsive } = useResponsive() | ||
| const { | ||
| datasetId: myDatasetId, | ||
| error, | ||
| loading, | ||
| getDataset | ||
| } = useDatasetManager() | ||
| const { isProcessingDataset, polledDatasetState, pollDatasetId } = | ||
| usePollDatasetStatus() | ||
| const [dataset, setDataset] = useState({}) // dataset currently displayed on the page | ||
|
|
||
| const getDatasetFromQuery = async (id) => { | ||
| const response = await getDataset(id) | ||
| setDataset(response) | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| // redirects users to /download if datasetId matches My dataset ID | ||
| if (datasetId === myDatasetId) push('/download') | ||
|
|
||
| getDatasetFromQuery(datasetId) | ||
| pollDatasetId(datasetId) // sets a processing datasets for polling | ||
| }, [datasetId]) | ||
|
|
||
| useEffect(() => { | ||
| // swaps dataset to the last fetched polledDatasetState | ||
| // (only if the dataset ID in URL was being processed) to update | ||
| // DatasetPageHeader | ||
| if (!isProcessingDataset && polledDatasetState) { | ||
| setDataset(polledDatasetState) | ||
| } | ||
| }, [isProcessingDataset, polledDatasetState]) | ||
|
|
||
| if (!pageRendered) return null | ||
|
|
||
| if (start) { | ||
| return ( | ||
| <FixedContainer> | ||
| <StartProcessing dataset={dataset} /> | ||
| </FixedContainer> | ||
| ) | ||
| } | ||
|
|
@@ -82,52 +83,41 @@ export const Dataset = ({ query }) => { | |
| ) | ||
| } | ||
|
|
||
| if (loading) { | ||
| return ( | ||
| <Box align="center" fill justify="center" margin={{ top: 'large' }}> | ||
| <Spinner /> | ||
| </Box> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <FixedContainer> | ||
| <Box> | ||
| {dataset?.data && ( | ||
|
There was a problem hiding this comment. I think we can get rid of this check as well there should only be 2 states we care about. Loading and loaded. I would think that if dataset.data is undefined then loading would be true. There was a problem hiding this comment. It has been cleaned up in 41e8260 👍 |
||
| <> | ||
| <DatasetPageHeader dataset={dataset} /> | ||
| <Row | ||
| border={{ side: 'bottom' }} | ||
| pad={{ bottom: setResponsive('medium', 'small') }} | ||
| > | ||
| <Box> | ||
| <MoveToDatasetButton dataset={dataset} /> | ||
| </Box> | ||
| <Row | ||
| gap={setResponsive('medium', 'small')} | ||
| margin={{ top: setResponsive('medium', 'none') }} | ||
| > | ||
| <ShareDatasetButton dataset={dataset} /> | ||
| <DownloadDatasetButton dataset={dataset} /> | ||
| </Row> | ||
| </Row> | ||
| <FilesSummary dataset={dataset} /> | ||
| <DatasetSummary dataset={dataset} /> | ||
| <DatasetDetails dataset={dataset} isImmutable /> | ||
| </> | ||
| )} | ||
| </Box> | ||
| </FixedContainer> | ||
| ) | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should replace this with a helper that wraps the actual meaning of these state attributes. They are not the clearest names on the API. We can't really change them very easily there because external scripts might be using them so here we should just abstract away the specifics into the actionable states.
I think you can clean this up but the important part is that logically only one of these states can be true at one #time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
David, as chat on Slack (and noted in here as well #364 (comment)), I've filed a dedicated new issue (#370), for adding
getDatasetStatehelper and refactoring the components that use dataset states.This should make the review easier while keeping the file changes minimal and reducing the merge conflicts (the PR for it is also ready (#373). Thank you David!