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.

Sign up for GitHub

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

fix(feed): boundaries and zoom for map feed #1399

Merged
merged 10 commits into from
Dec 6, 2024
Merged

fix(feed): boundaries and zoom for map feed #1399

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3448435
fix: max bound set for whole map
sujal-into Nov 19, 2024
3529fa4
removed the latLng BooundExpression type
sujal-into Nov 20, 2024
773a6c6
Replaced the latlngBoundExpression type with LatlngBoundLietral from …
sujal-into Nov 20, 2024
b63bc03
fix: responsive zoom level
sujal-into Nov 20, 2024
b42b56d
fix: added the map half bound
sujal-into Nov 25, 2024
066f8cc
fix: extraced the useUpdateMinZoom hooks from the DynamicMinZoom comp…
sujal-into Nov 25, 2024
8835c32
lint-fix: removed the unused imports
sujal-into Nov 25, 2024
4c743ac
fix: zoom and boundaries for web Map Feed
clegirar Dec 3, 2024
7048c47
Merge branch 'main' into bugfix/zoom-out-bounds
clegirar Dec 5, 2024
49d4069
Merge branch 'main' into bugfix/zoom-out-bounds
clegirar Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 53 additions & 2 deletions packages/components/socialFeed/Map/Map.web.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import "./styles.css";
import "leaflet/dist/leaflet.css";
import { DivIcon, LatLngBounds, point, PointExpression } from "leaflet";
import {
DivIcon,
LatLngBounds,
LatLngBoundsLiteral,
point,
PointExpression,
} from "leaflet";
import {
Dispatch,
FC,
Expand Down Expand Up @@ -30,6 +36,7 @@ import {
getMapPostIconColorRgba,
getMapPostIconSVGString,
MAP_LAYER_URL,
MAP_MAX_BOUND,
} from "@/utils/feed/map";
import { zodTryParseJSON } from "@/utils/sanitize";
import {
Expand Down Expand Up @@ -103,6 +110,43 @@ const MapManager = ({
return null;
};

const DynamicMinZoom = ({
setMinZoom,
}: {
setMinZoom: Dispatch<SetStateAction<number>>;
}) => {
const map = useMap();

useEffect(() => {
const lat = 45;
const lng = 90;

const adjustedWorldMapBounds: LatLngBoundsLiteral = [
[-lat, -lng],
[lat, lng],
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is independent of the Component so we should put it outside of the component and we can use UPPER_CASE to define the constants

Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't follow this convention, please use normal variable naming as in js, there is no real constants

we can put it outside of the func though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed here: b42b56d

Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't follow this convention, please use normal variable naming as in js, there is no real constants

we can put it outside of the func though

IMHO, if it's a constant and declared outside of function scope, it should be a UPPER_CASE.
if it's just a keyword const and declared in function scope, it's ok to define it with normal case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

const in js don't exist
Screenshot 2024-12-04 at 18 02 19

Copy link
Collaborator

Choose a reason for hiding this comment

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

this makes refacto harder if the variable stops being a constant according to your definition
also we don't have a way to enforce this rule so this means more review work

Copy link
Collaborator

@n0izn0iz n0izn0iz Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the rust language it make more sense because constant are real constant and thus when you see UPPER_CASE, you have some guarantees. Even then it doesn't have much value IMO but at least it can be verified by tooling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can vote on this


const updateMinZoom = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not defined the function inside of useEffect which makes the code less readable.

I suggest sth like

useEffect(() => {
    !!map && updateMinZoom(map);
    .......
}, [[map, setMinZoom])

Copy link
Collaborator

@n0izn0iz n0izn0iz Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree, we followed this in berty and it made the code very hard to maintain IMO

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can extract a hook though if you really want

const useUpdateMinZoom = (map) => {
    useEffect(...)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed here: 066f8cc

if (map) {
const calculatedZoom = map.getBoundsZoom(adjustedWorldMapBounds, false);
setMinZoom(calculatedZoom);
map.setMinZoom(calculatedZoom);
map.setZoom(calculatedZoom);
}
};

updateMinZoom();

window.addEventListener("resize", updateMinZoom);

return () => {
window.removeEventListener("resize", updateMinZoom);
};
}, [map, setMinZoom]);

return null;
};

export const Map: FC<MapProps> = ({
consultedPostId,
style,
Expand All @@ -111,6 +155,7 @@ export const Map: FC<MapProps> = ({
}) => {
const selectedNetworkId = useSelectedNetworkId();
const [bounds, setBounds] = useState<LatLngBounds | null>(null);
const [minZoom, setMinZoom] = useState(2);

// Fetch the consulted post
const { post: consultedPost } = usePost(consultedPostId);
Expand Down Expand Up @@ -197,6 +242,7 @@ export const Map: FC<MapProps> = ({
width: "100%",
height: "100%",
alignSelf: "center",
maxHeight: 1000,
},
style,
]}
Expand All @@ -205,8 +251,12 @@ export const Map: FC<MapProps> = ({
center={
consultedPostLocation || creatingPostLocation || DEFAULT_MAP_POSITION
}
zoom={12}
zoom={minZoom}
minZoom={minZoom}
attributionControl={false}
maxBounds={MAP_MAX_BOUND}
maxBoundsViscosity={1.0}
style={{ width: "100%", height: "100%" }}
>
{/*----Loads and displays tiles on the map*/}
<TileLayer noWrap attribution="" url={MAP_LAYER_URL} />
Expand Down Expand Up @@ -280,6 +330,7 @@ export const Map: FC<MapProps> = ({
creatingPostLocation={creatingPostLocation}
consultedPostLocation={consultedPostLocation}
/>
<DynamicMinZoom setMinZoom={setMinZoom} />
</MapContainer>
</View>
);
Expand Down
5 changes: 5 additions & 0 deletions packages/utils/feed/map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { LatLngBoundsLiteral } from "leaflet";
import { FunctionComponent } from "react";

import unknownSvg from "@/assets/icons/question-gray.svg";
Expand All @@ -14,6 +15,10 @@ export const MAP_LAYER_URL = `https://{s}.tile.jawg.io/jawg-dark/{z}/{x}/{y}{r}.

// Paris baguette
export const DEFAULT_MAP_POSITION: CustomLatLngExpression = [48.8566, 2.3522];
export const MAP_MAX_BOUND: LatLngBoundsLiteral = [
[-90, -180], // South-West corner
[90, 180], // North-East corner
];

const musicPostSvgString = `<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">
<g filter="url(#filter0_di_22136_173395)">
Expand Down
Loading