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

Integrate Geo-Copilot to VEDA UI Exploration Page #1173

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

Conversation

xhagrg
Copy link

@xhagrg xhagrg commented Sep 25, 2024

Description of Changes

This PR introduces components in the exploration page required for interaction with Geo-CoPilot backend being developed by our Microsoft Counterparts. Most of the interactions done in exploration page can now be done with the help of geo-copilot, mainly loading datasets, comparing dates for datasets, and running analysis.

Notes & Questions About Changes

This PR mostly tackles the basic functionalities of the co-pilot. There are more interactions planned, and will be updated in the next PRs to come.

Validation / Testing

  • The new components touch all of the existing features. However, it should not affect any of those functionality
  • Interaction with Geo-Copilot should not result in un-restricted loading bar. (any interaction should either lead to error message or message from the co-pilot)
  • Manual interaction should not affect the interaction with geo-copilot.

Copy link

netlify bot commented Sep 25, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit daf659d
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/672cf085eda08800084fd23e
😎 Deploy Preview https://deploy-preview-1173--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.

@xhagrg xhagrg requested a review from hanbyul-here October 9, 2024 13:57
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 it. It is such a fun feature and I like seeing the methods we built are being used by geo-copilots to dynamically add datasets and run analysis 🤩

I could test and validate the functionalities you described in the pr. The main problem I can spot now is AOIs are behaving unexpectedly. ex. already-drawn AOIs are still there even after geo-copilots finished the task, and pre-defined AOI selector (the dropdown on the left-top with US States as options) doesn't get reset after AOI is gone. Of course this change was not introduced by your change, it is more like it highlights the problem we already had. - We are currently working on the improvements now (#1207). I am not sure if that will fix all the issues, but I hope it helps future iterations.


const [timeDensity, setTimeDensity] = useState<TimeDensity | null>(null);

const [map, setMap] = useState<MapboxMap | MapRef>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I had tried to use useMaps, but probably because of some race condition, the maps are not ready/accessible when trying to use the geo-copilot panel. I will check again and see if I can use the ref again.

@@ -89,33 +111,93 @@ function ExplorationAndAnalysis(props: ExplorationAndAnalysisProps) {
};
}, [resetAnalysisController, setUrl]);

const mapPanelRef = useRef<any>(null);
Copy link
Collaborator

@hanbyul-here hanbyul-here Oct 22, 2024

Choose a reason for hiding this comment

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

Have you tried using utility methods that react-resizable-panel exposes and using one ref? https://www.npmjs.com/package/react-resizable-panels#can-a-attach-a-ref-to-the-dom-elements

Copy link
Author

Choose a reason for hiding this comment

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

I hadn't seen this. Thank you, @hanbyul-here . that definitely helps make the code a little cleaner.

Copy link
Author

Choose a reason for hiding this comment

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

@hanbyul-here I checked on this, and getPanelElement is not available in the current version of react-resizable-panels. will keep the changes for now, and will upgrade later.

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