-
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
Create minimal partial data layer scaffold starting off with Data Catalog for VEDA2 Refactor #893
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 looked over this but I don’t fully understand these changes and how it will evolve in the future.
I left two questions in the code but my main question is about the data-layer/datasets
module: How will this be used in the future? I’m assuming there will be more complexity in the module at some point; what functionality are we planning to implement in the module in the future?
tsconfig.json
Outdated
@@ -8,7 +8,8 @@ | |||
"$components/*": ["app/scripts/components/*"], | |||
"$styles/*": ["app/scripts/styles/*"], | |||
"$utils/*": ["app/scripts/utils/*"], | |||
"$context/*": ["app/scripts/context/*"] | |||
"$context/*": ["app/scripts/context/*"], | |||
"$data-layer/*": ["data-layer/*"] |
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 keep the repo clean, could we move this into from the repository root to a subdirectory like src?
"$data-layer/*": ["data-layer/*"] | |
"$data-layer/*": ["src/data-layer/*"] |
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.
☝️ Can we put data-layer
in app/scripts
? That is where all the 'scripts' are right now.
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.
@oliverroick @hanbyul-here should I move it under app/scripts/*
or create a new directory to do src/data-layer/*
? I will say I personally dont understand why we have a dir under app named /scripts
and why it was named that but maybe we can revisit naming later.. lmk which you both prefer here
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 we want to keep it all in the same place it should go to app/scripts
. But I agree, we should revisit that at some point, this app/scripts
convention we're using in some projects is odd.
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.
Updated, can I get a re-review?
@oliverroick thanks for taking a look, so for the This PR is just moving the veda virtual modules datasets under the data-layer to establish a single source where our components would now call from. If we updated this logic or removed this virtual module, ideally it wouldn't affect the components because of this abstraction. I was also thinking we would ideally have much of our fetching and hooks logic live here, doesn't have to be explicitly this file (file names and structure will change, I just created this file for now to just start). Then the rest of the PR is just me creating a wrapper container for the DataCatalog Feature Component that interacts with the data layer to provide the data to this feature component. This is just to support the current architecture though. The template instance wouldn't need this wrapper container i'm creating as they would have their own page views that would interact with the data layer and pass directly to the feature component. Does this help answer you Q? And any ideas from your end though on how to do this better though? Since this is the scaffold, right now is a great time to start talking about it! 🙏🏼 |
Thanks @sandrahoang686 that's clear now. As for the work you've put in the PR, that looks all good to me. What we need to consider though is how the changes you plan in future relate to and affect to the discussion/spike for the parcel builder the @hanbyul-here is working on. From what Daniel is saying in the ticket, the parcel builder not only parses the content but also generates option lists that allow filtering for content. You mentioned data fetching in hooks, were you planning that this happens on-the-fly or are we still going to use a fair amount of pre-processing? |
@oliverroick yeah I didn't realize how much the parcel resolver was doing... and I haven't thought about it. I'm thinking though and if I understood your question, seems like the parcel resolver is generating the list of taxonomies and datasets during build time but i'm thinking can we do something similar with something like NextJs's pre-rendering and static generation? So both? We initially fetch at build time with something like What do you think? |
e005df2
to
7c80c2a
Compare
## 🎉 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:** #892 Data Catalog now takes in datasets as a prop and to support the current architecture, a wrapper container was created to interact with the data-layer to provide data to this feature component ([PR#893](#893)). Note: The template instance wouldn't need this wrapper container as they would have their own page views that would interact with the data layer. Page layout / styling is also removed from Data-catalog component into the wrapper ([PR#930](https://github.com/NASA-IMPACT/veda-ui/pull/930/files)). @j08lue @aboydnw @faustoperez The data-catalog feature component is now just what is highlighted in the red box. So when this feature is imported, they would only get what i've highlighted here. Page layout and headers would be something the actual page manages now. Does this seem fine to you both? ![Screenshot 2024-05-09 at 3 14 00 PM](https://github.com/NASA-IMPACT/veda-ui/assets/30272083/812cb9a9-15da-4862-bb72-6f70f05f9a82) ### Description of Changes * Allow card override * Clean up unused / dead code ### Notes & 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"}_
Related Ticket:
Description of Changes
This is a very minimal partial implementation of the data layer scaffolding to start the conversation and work on breaking out the feature components which will involve moving data logic out of the components into the data layer. This PR moves the datasets dependency out from the Data Catalog component to help kick off breaking out this Feature Component to be data agnostic
This Data Layer scaffold will be a WIP throughout the refactor and scaffold WILL EVOLVE. This is just to define a current place where data logic can be migrated too to start core feature breakouts
Notes & Questions About Changes
@hanbyul-here thoughts?
Validation / Testing