-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle analysis/define datasets w/o collection summaries #786
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
app/scripts/components/analysis/define/use-stac-collection-search.ts
Outdated
Show resolved
Hide resolved
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 left some thoughts about type!
@@ -35,7 +35,7 @@ export interface TimeseriesDataResult { | |||
isPeriodic: boolean; | |||
timeDensity: TimeDensity; | |||
domain: string[]; | |||
timeseries: TimeseriesDataUnit[]; | |||
timeseries?: TimeseriesDataUnit[]; // NOTE: Summaries on collections will not always be available |
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.
To make the type useful, we seem to need a new type. The components like chart-card
has to have timeseries data, so it doesn't seem to reflect is actual usage to type timeseires
as an optional. (Also the data with summaries and wo summaries having the same type is confusing, like this line : https://github.com/NASA-IMPACT/veda-ui/pull/786/files#diff-73d60515e81c1007b36e13ea2886f3b4e1457746381934d40af7802e39841022R182)
How about leaving the TimeseriesDataResult
as it is, but making TimeseriesDataWithMissingSummaries
(I just made the variable name up) and use this type explicitly for the data without summaries?
More food for thought: when the data is missing summaries, we don't seem to need any attributes from the collection?
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 like the idea of being explicit but if we dont seem to need any attributes from the collection when summaries is missing from the data, then we can probably just use existing type DatasetLayer
. Will make this change
app/scripts/components/analysis/define/use-stac-collection-search.ts
Outdated
Show resolved
Hide resolved
I made a pr against this branch with some ts change suggestions: #797 Please take a look! |
…atasets explicitly (#797) I meant to just see if I can fix a ts warning (that timeseries can be undefined ) and then ended up with a bigger change -_-; - I made an explicit type for `TimeseriesMissingSummaries` and also `InvalidDatasets` which can be both DatasetWithTimeseriesData or DatasetMissingSummaries.
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.
!
https://github.com/NASA-IMPACT/veda-ui/releases/tag/v4.1.0 ## 🎉 Features - Experimental CMR Layer: #805 ## 🚀 Improvements - Style - Documentation - Rename critical error page: #806 - Add back button on Exploration page: #779 - Add ts, lint check for precommit hooks: #788, #809 - "skip to main" button for navigation: #808 ## 🐛 Fixes - Return datasets even when there is a dataset without summaries: #786 - Show all the datasets on Data Catalog page: #837 - Block Map user defined position fix: #784 - Geocoder centering on various projecctions: #826
## 🎉 Features - Optional media attributes for layers: #843 - Add custom javascript injection #846 - ADR for V2 Refactor: #875 ## 🚀 Improvements - E&A imporvement. Related tickets - Layer select modal: #845 - Connect dataset information on layer: #821 - Layer info modal: #849 - Update data layer card: #851 - Hidden layers: #867 - Fast follow-ups: #851 , #862, #863, #860 - PR template: #880 ## 🐛 Fixes - Return datasets even when there is a dataset without summaries: #786 - Show all the datasets on Data Catalog page: #837 - Block Map user defined position fix: #784 - Geocoder centering on various projecctions: #826 - Wording, typo: #869 #854, #874, - Fix yaxis labeling: #883
This aims to solve ticket here
Exploration and Analysis page seems like it already handles summaries are not available here and here
For Analysis/Results: We dont have to worry because users would not be able to select the dataset without summaries any longer with this fix
Loom Local Validation:
https://www.loom.com/share/d0b132ca88b242dc8014e277495d6746
UPDATE TO FLAG COMMENT: