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

[FEATURE BRANCH] 902 E&A breakout #1154

Merged
merged 39 commits into from
Nov 14, 2024
Merged

[FEATURE BRANCH] 902 E&A breakout #1154

merged 39 commits into from
Nov 14, 2024

Conversation

sandrahoang686
Copy link
Collaborator

@sandrahoang686 sandrahoang686 commented Sep 17, 2024

Related Ticket: #902

Description of Changes

Related NextJs PR/Branch: developmentseed/next-veda-ui#4

GHG Preview with this branch pulled in generated here => US-GHG-Center/veda-config-ghg#659

Iteration 1 - Issue #902, changes by @sandrahoang686

  • Exposed ExplorationAndAnalysis component
  • Exposed timelineDatasetsAtom to be used NextJs side
  • Exposed & Moved/decoupled DatasetSelectorModal out of ExplorationAndAnalysis. ExplorationAndAnalysis only needs to know the dataset layers to be displayed. DatasetSelectorModal or whatever layer selection method should be controlled at the parent level above ExplorationAndAnalysis.
  • Commented out certain controls that were breaking ExplorationAndAnalysis in NextJs instance
    • AnalysisMessageControl
    • CustomAoIControl

Notes & Questions About Changes

{Add additonal notes and outstanding questions here related to changes in this pull request}

Validation / Testing

  • Validate that Datalayer Selection Modal works as expected
  • Validate that E&A features work as expected - aois, analyzation, adding/removing cards
  • Validate that all card linking works as expected

Copy link

netlify bot commented Sep 17, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 387401f
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6734be5cd9905a0008cc3c36
😎 Deploy Preview https://deploy-preview-1154--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
Copy link
Collaborator

hanbyul-here commented Oct 22, 2024

🤚 I am having hard time running the branch on Next repo, getting an error from the line that uses startOfYear from date-fns > 91 | .domain([startOfYear(now), endOfYear(now)]) - Does anybody have insights on what I should do?

@dzole0311
Copy link
Collaborator

🤚 I am having hard time running the branch on Next repo, getting an error from the line that uses startOfYear from date-fns > 91 | .domain([startOfYear(now), endOfYear(now)]) - Does anybody have insights on what I should do?

Yes, I had the same problem and fixed it in my map controls branch: https://github.com/NASA-IMPACT/veda-ui/pull/1205/files#diff-c01e76db4162d7ec2d29c4cd98dcadb5826d1ead020f96f94159a72d77d7b3b5R6

@sandrahoang686
Copy link
Collaborator Author

@hanbyul-here @dzole0311 yep, i highlighed that in an earlier PR - seems to be a known issue with parcel #1046 (comment)

**Related Ticket:** #1154

### Description of Changes
* Fixed to have changes from
https://github.com/NASA-IMPACT/veda-ui/pull/1231/files# which I
accidentally removed when resolving conflicts in main feature branch
* Remove `onClick` from `LinkProperties`
* When something is a link, it doesn't need an onClick event... The
onClick event should be placed on the `Card` Component instead of link
props
* LinkProperties should not be required for the card component. Only
when the card re-routes would it need the linkProperties. For example,
cards on the dataset selector modal dont re-route and we shouldn't have
to pass in linkProps
* Remove try/catch around aoi area/point selection logic and into local
state with useEffect

### Note
Built `v5.9.1-ea.0` off of these changes
Update: Bulit `v5.9.1-ea.1` has been built

### Validation / Testing
* Please make sure Dataset Layer Selection modal and E&A works as
expected
* Please make sure all cards in the dashboard are working as expected
* Make sure external cards link externally and have the external badge
   * Make sure internal cards link internally correctly
   * Make sure Dataset Layer Selection modal selects cards as expected

**NextJs Preview with updated version:**
developmentseed/next-veda-ui#4
@hanbyul-here
Copy link
Collaborator

hanbyul-here commented Nov 14, 2024

Leaving a note of UI changes that is not intended (I will add if I find more - also feel free to modify this comment if anybody finds anything.)

  • Mouse cursor does not show the hover state on the data selector modal on exploration page

Copy link
Collaborator

@dzole0311 dzole0311 left a comment

Choose a reason for hiding this comment

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

Most of the changes were already tested via the other PRs, so I did a quick smoke test and couldn't spot anything blocking.

@sandrahoang686
Copy link
Collaborator Author

sandrahoang686 commented Nov 14, 2024

@hanbyul-here thanks for catching, created an issue for that here as i dont think its blocking 👍🏼

Update: We actually want that behavior of the cursor change because we removed linking from datalayer selector cards so unfiling but kept placeholder ticket open

@sandrahoang686 sandrahoang686 merged commit a8311c2 into main Nov 14, 2024
14 checks passed
@sandrahoang686 sandrahoang686 deleted the 902-ea-breakout branch November 14, 2024 18:42
dzole0311 added a commit that referenced this pull request Nov 26, 2024
### 🚀 Improvements
* Exploration & Analysis core VEDA feature breakout
#1251
#1154
* Introduce `EnvConfigProvider` to simplify the injection of environment
variables from host applications like Next.js
#1253
* Remove React Router's `useNavigate` dependency for simplifying the
routing #1270

### 🐛 Fixes
* Fix an issue where card images fail to maintain the correct aspect
ratio #1265
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