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

feat:types for map #1856

Merged
merged 28 commits into from
Jan 24, 2024
Merged

feat:types for map #1856

merged 28 commits into from
Jan 24, 2024

Conversation

sunilsabatp
Copy link
Contributor

fix -> types error for maps

@vercel
Copy link

vercel bot commented Aug 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
planet-webapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2024 4:08am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
planet-webapp-temp ⬜️ Ignored (Inspect) Visit Preview Jan 23, 2024 4:08am

Copy link
Collaborator

@mohitb35 mohitb35 left a comment

Choose a reason for hiding this comment

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

@sunilsabatp Please go through my comments.

src/features/projects/components/maps/Markers.tsx Outdated Show resolved Hide resolved
src/features/user/RegisterTrees/RegisterTrees/DrawMap.tsx Outdated Show resolved Hide resolved
src/features/user/TreeMapper/components/Map.tsx Outdated Show resolved Hide resolved
src/features/user/TreeMapper/components/Map.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@mohitb35 mohitb35 left a comment

Choose a reason for hiding this comment

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

@sunilsabatp Please take a look at the open comments here and previous ones as well

Copy link
Collaborator

@prachigarg19 prachigarg19 left a comment

Choose a reason for hiding this comment

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

When I click on a plant location I get an application side error:
image

@prachigarg19
Copy link
Collaborator

In register trees, when I added a larger number like 60, I saw the following error:
image

@sunilsabatp
Copy link
Contributor Author

In register trees, when I added a larger number like 60, I saw the following error: image

I am not encountering this error, could you please give me a demo instead ?

Copy link
Collaborator

@prachigarg19 prachigarg19 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Shreyaschorge Shreyaschorge left a comment

Choose a reason for hiding this comment

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

Minor changes not merge-blockers

@@ -186,8 +191,12 @@ export default function ProjectsMap(): ReactElement {
</div>
{showDetails.show && (
<Popup
latitude={showDetails.coordinates[1]}
longitude={showDetails.coordinates[0]}
latitude={
Copy link
Contributor

Choose a reason for hiding this comment

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

On line number 161 onStateChange is not a part of the MapGL props

const [open, setOpen] = React.useState(false);
const buttonRef = React.useRef<HTMLButtonElement>(null);
const { embed, callbackUrl } = React.useContext(ParamsContext);

const handleClose = () => {
setOpen(false);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

In markerBackgroundColor parameter project implicitly has an 'any'

@@ -30,6 +30,7 @@ export default function PlantLocations(): ReactElement {
const { i18n, t } = useTranslation(['maps', 'common']);

const openPl = (pl: PlantLocationSingle | SamplePlantLocation) => {
console.log(pl, '==');
Copy link
Contributor

Choose a reason for hiding this comment

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

Clean up needed

@@ -231,7 +232,10 @@ export default function PlantLocations(): ReactElement {
{viewport.zoom > 14 && (
<div
key={`${pl.id}-marker`}
onClick={() => openPl(pl)}
onClick={() => {
console.log('I ran');
Copy link
Contributor

Choose a reason for hiding this comment

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

Clean up needed

@@ -100,6 +110,8 @@ export default function MapComponent({
width: '100%',
}}
>
{/* NOTE: this functionality does not seem to work locally using React 18.
To test, a temporary fix is to set `reactStrictMode=false` in next.config.js */}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no on props in DrawControl's DrawControlProps

@@ -105,65 +90,22 @@ export default function MapComponent({
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to disable eslint on line 86

@@ -1,21 +1,26 @@
import React, { ReactElement } from 'react';
import TransactionListLoader from '../../../../../public/assets/images/icons/TransactionListLoader';
import TransactionsNotFound from '../../../../../public/assets/images/icons/TransactionsNotFound';
import PlantLocation from './PlantLocation';
import styles from '../TreeMapper.module.scss';
import { useTranslation } from 'next-i18next';
import {
SamplePlantLocation,
Copy link
Contributor

Choose a reason for hiding this comment

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

Clean up needed

}
longitude={
showDetails?.coordinates ? showDetails?.coordinates[1] : 0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Cleanup needed for DetailsType

Copy link
Collaborator

@mohitb35 mohitb35 left a comment

Choose a reason for hiding this comment

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

LGTM

Further improvements are possible, but let's look into that in the future.

@mariahosfeld mariahosfeld merged commit 57699e9 into develop Jan 24, 2024
7 checks passed
@mariahosfeld mariahosfeld deleted the feature/type_for_map branch January 24, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants