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

Added the map feature [bug] #28

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

Conversation

Kingsuite
Copy link

image

@Kingsuite Kingsuite changed the title Added the feature [wrapping up on the pin features—the onHover projectDetails] Added the map feature WIP [wrapping up on the pin features—the onHover projectDetails] Nov 28, 2024
@schwepps schwepps self-requested a review November 29, 2024 09:35
@schwepps
Copy link
Member

Hey @Kingsuite ! I just checked your PR and I don't see any front dev in the file changes. Only config files modification.
Let me know if you need help.

@Kingsuite
Copy link
Author

Hey @Kingsuite ! I just checked your PR and I don't see any front dev in the file changes. Only config files modification. Let me know if you need help.

  • One map is displayed before the list of assets
  • One pin with the name of the asset is displayed for each project.
  • The pin is the svg image of the project.
  • When I hover the pin, the image becomes bigger to be able to read the information (I can't really tell because of pin alignment issues)
  • When I click on the pin or the image, i'm redirected to the project page

I have updated the files for reproduction. But I noticed some bug with integrating the map in this environment:

  • I tested on refresh CodeSandBox, and everything seems to be working fine & the pins align well.
  • My geocodeLocation() function returns the coordinates which I have tested on Google Map & on a React app with this map library and everything works fine.
    WhatsApp Image 2024-11-28 at 7 34 06 PM

@Kingsuite Kingsuite changed the title Added the map feature WIP [wrapping up on the pin features—the onHover projectDetails] Added the map feature WIP [bug] Nov 30, 2024
@Kingsuite Kingsuite changed the title Added the map feature WIP [bug] Added the map feature [bug] Nov 30, 2024
@Kingsuite
Copy link
Author

@schwepps have you able to go through it?

@schwepps
Copy link
Member

schwepps commented Dec 2, 2024

@Kingsuite I'm going to review it this morning.

Copy link
Member

@schwepps schwepps left a comment

Choose a reason for hiding this comment

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

@Kingsuite I have added a few comments.
The map is displayed correctly, the coordinates are correctly fetched but the markers are not displayed on the map.
Could you check that please?
Also no need to change the marker on hover if too complicated.


function CustomMarker({ item }: CustomMarkerProps) {
const id = item?.name.toString().toLowerCase().replace(' ', '_');
const image = item?.image_data ? item?.image_data.replaceAll('%23', '#') :
Copy link
Member

Choose a reason for hiding this comment

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

I have checked locally and there is no image_data in item. The attribute is called image.


return (
<>
{console.log(item, "WTF")}
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the console.log?


})
);
console.log(updatedData, "Coordinates");
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the console.log?

const mapRef = useRef<maplibregl.Map | null>(null);
useEffect(() => {
if (mapData) {
console.log(filterByName(mapData));
Copy link
Member

Choose a reason for hiding this comment

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

Is there some code missing here instead of the console.log?

Copy link
Author

Choose a reason for hiding this comment

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

No nothing was missing here I was testing the filter function


return mapData.length !== 0 ? <RMap
initialCenter={locations[0] == undefined ? [0, 0] : [locations[0].coordinates[0], locations[0].coordinates[1]]}
style={{ width: '100%', height: "800px", overflowY: "hidden" }}
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the map height a bit smaller like 16:9 ratio?

@Kingsuite
Copy link
Author

@schwepps I will revert shortly. I am working on it

@Kingsuite
Copy link
Author

Kingsuite commented Dec 2, 2024

markers are not displayed on the map.

For this I am still figuring out why it's not working. Because I started afresh a React app with these same library, and it worked perfectly. What if there's an overhead style interfering?

@julienbrs julienbrs requested review from schwepps and removed request for schwepps December 3, 2024 09:43
@Kingsuite
Copy link
Author

@schwepps GM, have you had time to review the updates?

@schwepps
Copy link
Member

schwepps commented Dec 3, 2024

@Kingsuite I think the issues with the markers and the projects images below are due to not having a unique key for each element.
Each project image should have a unique key and the markers, if they are using the same images should also be unique by adding a prefix or a suffix to the key.

@Kingsuite
Copy link
Author

@Kingsuite I think the issues with the markers and the projects images below are due to not having a unique key for each element. Each project image should have a unique key and the markers, if they are using the same images should also be unique by adding a prefix or a suffix to the key.

I have tried the key, but not so much has changed. I will push in my new observations.

@Kingsuite
Copy link
Author

Kingsuite commented Dec 3, 2024

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