NASA-IMPACT / veda-ui

Frontend for the Dashboard Evolution project
Other
25 stars 5 forks source link

Various fixes for E&A #1229

Closed hanbyul-here closed 2 weeks ago

hanbyul-here commented 3 weeks ago

Related Ticket: https://github.com/NASA-IMPACT/veda-ui/issues/1220

Description of Changes

This PR is WIP, opening in case it helps anybody. (That says, please don't spend time reviewing the changes in this PR yet, but it will be really helpful if you can validate that this branch can run with Next instance with stability.)

Notes & Questions About Changes

I've noticed E&A still doesn't run with stability in Next instance. I traced down all the possible errors that I can spot. That includes

Fixed in this PR

Not fixed in this PR yet

Validate & Test

Please test this branch with test-branch Next branch : https://github.com/developmentseed/next-veda-ui/tree/test-branch - It will still lack of some functionalities, but hopefully the e&a page doesn't return abrupt errors anymore.

netlify[bot] commented 3 weeks ago

Deploy Preview for veda-ui ready!

Name Link
Latest commit f5bb96cfb0899c7154546de3900e91e771d5c883
Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/672b8830270a8d000806b617
Deploy Preview https://deploy-preview-1229--veda-ui.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

hanbyul-here commented 3 weeks ago

This PR handled the most of errors, except that the timelinedataset hydration through URL parameter not working. To simply describe the issue, we are using the dataLayers from Veda faux module (already compiled, and be used like a dependency) to reconcile the datasets from URL parameters. : https://github.com/NASA-IMPACT/veda-ui/blob/main/app/scripts/components/exploration/atoms/datasets.ts#L54

We would need to change how we approach static datasets for this part - I think one of the possible solution is to make the static datasets also an atom and hydrate it so it can be used inside of other atoms. Since this will require a bit more thoughts, I propose this PR to be merged first, and tackle this problem separately.

I also wonder if we need to consolidate how other components pass static datasets as props to standardize the dataflow once the issue above is resolved.

sandrahoang686 commented 2 weeks ago

Ran this locally with nextJs and test-branch and looks good enough to merge, I didn't get to do an in-depth code review but dont want to block this. We can look at this code as a whole in the feature branch though and be thorough before merging that feature branch πŸ‘πŸΌ cc @hanbyul-here @dzole0311