[DT-1077] typescriptify datasetstatistics and fix #2757
Conversation
s-rubenstein
requested review from
pshapiro4broad and
snf2ye
and removed request for
a team
| @@ -1,6 +0,0 @@ | |||
|
|
|||
There was a problem hiding this comment.
This file is side by side with a models.js file, and the capitalization being the only difference was causing issues. Given this was only used in one place I just moved it.
s-rubenstein
force-pushed
the
sr/dt-1077-typescriptify-datasetstatistics-and-fix
branch
from
1827bc8 to
51defb4
Compare
|
|
| {extract('Dataset Description') || dataset?.description || dataset?.study?.description || 'N/A'} | ||
| {extract('Dataset Description') || dataset?.study?.description || 'N/A'} |
There was a problem hiding this comment.
Why is dataset?.description not needed here?
There was a problem hiding this comment.
I think that was an error, the type Dataset doesn't have a description field. Maybe the API changed at some point and this was kept around for backward compatibility?
There was a problem hiding this comment.
Its not actually a field on the typed object! I don't know why it was there before.
| const [dars, setDars] = useState(); | ||
| const [datasetId, setDatasetId] = useState<number>(); | ||
| const [dataset, setDataset] = useState<Dataset>(); | ||
| const [dars, setDars] = useState<any>(); |
There was a problem hiding this comment.
Is it possible to type dars as well?
There was a problem hiding this comment.
It looks like it wouldn't be too hard, although this type doesn't exist yet in duos-ui. The Java type is a List of DarMetricsSummary
There was a problem hiding this comment.
| const extract = useCallback((propertyName) => { | ||
| const property = find({propertyName})(dataset.properties); | ||
| const extract = useCallback((propertyName: string) => { | ||
| const property = find({propertyName})(dataset?.properties) as DatasetProperty; |
There was a problem hiding this comment.
Would it be too ambitious to add types to the Ajax APIs too? If there's a way to incrementally add types to just the APIs used here that would be an improvement.
There was a problem hiding this comment.
Ajax often has types, but extract doesn't because properties is ambiguous.
|
|
||
| const LINE = <div style={{borderTop: '1px solid #BABEC1', height: 0}}/>; | ||
|
|
||
| export default function DatasetStatistics(props) { | ||
| enum AccessManagement { |
There was a problem hiding this comment.
Should this be moved to Model.ts? Or this only an UI concept and not part of an API model?
There was a problem hiding this comment.
It is not a part of the API model I believe. Or at least, it was in a separate file and model.js is a generated file. Model.ts was conflicting because of the import conflicts in js having issues with casing
| {extract('Dataset Description') || dataset?.description || dataset?.study?.description || 'N/A'} | ||
| {extract('Dataset Description') || dataset?.study?.description || 'N/A'} |
There was a problem hiding this comment.
I think that was an error, the type Dataset doesn't have a description field. Maybe the API changed at some point and this was kept around for backward compatibility?
s-rubenstein
deleted the
sr/dt-1077-typescriptify-datasetstatistics-and-fix
branch
Addresses
I noticed the issue appeared to be based on properties missing, so I added a test with no additional properties that still renders.
I opted not to put in the additional effort to typescriptify the test, because test files are much less impact as js vs ts, and if we decide to move unit tests to jest eventually, I wanted to prioritize effort.
Summary
Have you read Terra's Contributing Guide lately? If not, do that first.