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 cleanup on EA breakout feature branch #1251

Merged
merged 7 commits into from
Nov 13, 2024

Conversation

sandrahoang686
Copy link
Collaborator

@sandrahoang686 sandrahoang686 commented Nov 11, 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

Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 1ab1a3f
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6733b520caf0bf0008f52a9d
😎 Deploy Preview https://deploy-preview-1251--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.

@sandrahoang686 sandrahoang686 changed the title Add is LinkExternal back that was removed from conflict resolution Various cleanup on EA breakout feature branch Nov 11, 2024
@sandrahoang686 sandrahoang686 marked this pull request as ready for review November 11, 2024 21:43
@sandrahoang686
Copy link
Collaborator Author

sandrahoang686 commented Nov 11, 2024

@dzole0311 I had to resolve a good amount of conflicts in the original Feature branch 902-ea-breakout and had missed some things which I then fixed alongside this PR. Particularly the code related to your external link hotfix (look at description). If you could check that I didn't miss anything though? Also, if there is anything in the main feature branch PR that you think needs to be cleaned up more, please feel free to add to this PR with those fixes. 🙏🏼

@sandrahoang686
Copy link
Collaborator Author

sandrahoang686 commented Nov 12, 2024

LinkProperties are sprinkled in many places prop drilled quite deep, also the work around to support instances since they use the card component is just not ideal, we should probably update their code to pass in linkProps.. but I think we should spend more time looking at the navigation more holistically sometime in the future...

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

🙇 thanks for doing the extra work.

Adding my 2 cents to LinkProperties: Now that I think, what we should have done is instead of pathAttributeKeyName, we should have just made LinkElementProps and make it take whatever props that the component requires since we can't really predict what kind of props that LinkElement will need. 🤔 We can def separate the issue but what do you think?

--- a few hours later ---

Ah, I realized that even if we have LinkElementProps, it can't really replace what pathAttributeKeyName does since the LinkElement level doesn't know the path yet 😞

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.

  1. Hiding the external badge works as on prod ✅ Found a minor, non-urgent issue which I will address in another PR
  2. Tested card functionality and everything worked as expected ✅ The only change I noticed is that the cursor no longer shows as a "pointer" when hovering over E&A Catalog Cards due to the empty link props (which seems appropriate for this case)

@aboydnw
Copy link
Contributor

aboydnw commented Nov 12, 2024

I also did some manual testing and couldn't find any issues with this pr/preview link. looks good to me ✅

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

🙇 Thanks for working on this!

@sandrahoang686 sandrahoang686 merged commit 387401f into 902-ea-breakout Nov 13, 2024
9 checks passed
@sandrahoang686 sandrahoang686 deleted the 902-ea-breakout-fixes branch November 13, 2024 14:57
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.

4 participants