Skip to content
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

Various fixes for E&A #1229

Merged
merged 10 commits into from
Nov 6, 2024
Merged

Various fixes for E&A #1229

merged 10 commits into from
Nov 6, 2024

Conversation

hanbyul-here
Copy link
Collaborator

@hanbyul-here hanbyul-here commented Oct 30, 2024

Related Ticket: #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

  • export useAtom hook for timelinedataset (to remove the possibility of more than one jotai instances)
  • LinkProperties problem
  • MapboxDraw not added to the map yet when its method is called
  • Date value that gets fed to atom is not valid - this was because our polyfill for Array.last was not getting exported

Not fixed in this PR yet

  • TimelineDatasetAtom still depends on veda faux module datasets

I put temporary work-arounds for unsolved problems to validate that those are the errors that can interrupt Next instance.

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.

Copy link

netlify bot commented Oct 30, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit f5bb96c
🔍 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 hanbyul-here changed the title Etc fix Various fixes for E&A Oct 30, 2024
@hanbyul-here
Copy link
Collaborator Author

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.

@hanbyul-here hanbyul-here marked this pull request as ready for review November 4, 2024 14:58
@hanbyul-here hanbyul-here requested a review from AliceR November 4, 2024 15:22
return this[this.length - 1];
},
set: undefined
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but probably it's better to define it once in some kind of a polyfills.ts and use it / import it where needed.

@sandrahoang686
Copy link
Collaborator

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

@sandrahoang686 sandrahoang686 merged commit 7771378 into 902-ea-breakout Nov 6, 2024
8 checks passed
@sandrahoang686 sandrahoang686 deleted the etc-fix branch November 6, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants