-
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
Some datasets can only be analyzed with layers from the same source #913
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -178,6 +178,7 @@ export interface LayerInfo { | |||
*/ | |||
export interface DatasetData { | |||
featured?: boolean; | |||
sourceExclusive?: string; |
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.
If this is a binary value, would it make sense to make it boolean?
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 see the value is actually used, so it is not a boolean flag.
So, basically, you define here, which source value can exclusively be used. That is a bit redundant, though, because the is already defined under taxonomy
, so a boolean flag would be cleaner for this simple case where only the source of the current dataset can be exclusive.
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 I did this because some datasets look like they can have more than 1 source value like here. So to set the filter the easiest, i opted to do it like so. lmk if there are better ideas though and I will go change 🏃🏼♀️
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.
This is great for now, thanks! We can refine later, if we learn more about the use cases and needs for these restrictions - or find out we can avoid them with other solutions. 🤞
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.
The logic is as intended, great work! 💯
A little improvement might be if the filters from the layer selection modal would stick. Currently, they are reset when you add layers to the map, so if you add a layer from a "source-exclusive" dataset and then return to the layer selection modal to add more, you see all layers to select from, without the (source) filter. Changing that does not need to be in scope for this issue.
@@ -178,6 +178,7 @@ export interface LayerInfo { | |||
*/ | |||
export interface DatasetData { | |||
featured?: boolean; | |||
sourceExclusive?: string; |
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 see the value is actually used, so it is not a boolean flag.
So, basically, you define here, which source value can exclusively be used. That is a bit redundant, though, because the is already defined under taxonomy
, so a boolean flag would be cleaner for this simple case where only the source of the current dataset can be exclusive.
## 🎉 Features - Zoom in AOI, TOI when analysis is run in #906 - Add custom javascript injection #846 - ADR for V2 Refactor: #875 - Open all external links in a new tab in #870 - Include dataset id to filter layers in #910 - Some datasets can only be analyzed with layers from the same source in #913 - Create minimal partial data layer scaffold starting off with Data Catalog for VEDA2 Refactor in #893 - Add analysis preset in #921 ## 🚀 Improvements - Chart style improvement in #903 - Data Catalog enhancement with floating filter sidebar in #918 - Sum as statistics option in #925 - ## 🐛 Fixes - Sort featured stories based on publication date in descending order in #907 - Replace latency with temporalResolution in layer info in #898 - Add a workaround for Safari scroll problem in #909 - Handle empty result in #922
Related Ticket:
Closes #882 (comment)
Description of Changes
sourceExclusive
addedNotes & Questions About Changes
{Add additonal notes and outstanding questions here related to changes in this pull request}
Validation / Testing
{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}