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 Add E&A #4

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[FEATURE BRANCH] 902 Add E&A #4

wants to merge 12 commits into from

Conversation

sandrahoang686
Copy link
Contributor

@sandrahoang686 sandrahoang686 commented Sep 17, 2024

Linked Issue: NASA-IMPACT/veda-ui#1154

Related PRs:
#4

Will need to update library version when veda-ui feature branch PR is good to go

Validation:

  • Validate that Data Layer Selection Modal works as expected
    • Clicking cards are selected correctly and the selected cards persist in the E&A feature
  • Validate that the E&A feature is working correctly
    • AOIs
    • State persists when adding and removing layers
    • Analysis (Note**: 🐛 After checking - Analysis doesn't seem to be correctly, we might have to write a separate ticket for this) but I dont think this should block this from merging? lmk if preferred otherwise

Note:

  • 🐛 Its known that refereshing the E&A page with a dataset layer(s) gives an application error - bug ticket exists

Copy link

netlify bot commented Sep 17, 2024

Deploy Preview for veda-ui-next-test ready!

Name Link
🔨 Latest commit 63d4fab
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui-next-test/deploys/6733b93f4ee2d50008984b21
😎 Deploy Preview https://deploy-preview-4--veda-ui-next-test.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.

@aboydnw
Copy link
Member

aboydnw commented Nov 12, 2024

Not sure if this is due to this PR for E&A or if it's due to other things in the next.js instance, but I see some weird scrolling behavior where I can't scroll down all the way in the timeline panel at the bottom without moving the entire E&A tool up, see the video below

scrolling.in.next.js.EnA.mp4

Otherwise, I haven't found many issues. There is definitely different charting behavior on the data layers I have been able to generate analyses for (also see in the video) but I'm not sure if that's accidental or some addition to the charting library that I wasn't aware of?

sandrahoang686 added a commit to NASA-IMPACT/veda-ui that referenced this pull request Nov 13, 2024
**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
@sandrahoang686
Copy link
Contributor Author

sandrahoang686 commented Nov 13, 2024

@aboydnw thank you for validation 🙏🏼. Yeah, i left a comment about that in my description but haven't been able to dig into it just yet. Scrolling one is new to me, thanks for catching 👀. Would you be fine with ticketing those bugs to unblock merging these bigger feature branch PRs? We should definitely work on those as fast followups though

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.

4 participants