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

sujal-into · 2024-11-19T10:56:02Z

Description:
Set the zoom level to 3, so the map gets fit into the container, however, it doesn't show the whole map

netlify · 2024-11-19T10:56:16Z

✅ Deploy Preview for teritori-dapp ready!

Name	Link
🔨 Latest commit	`49d4069`
🔍 Latest deploy log	https://app.netlify.com/sites/teritori-dapp/deploys/6751a5030a7bc60008369179
😎 Deploy Preview	https://deploy-preview-1399--teritori-dapp.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-11-19T10:56:18Z

✅ Deploy Preview for testitori ready!

Name	Link
🔨 Latest commit	`8835c32`
🔍 Latest deploy log	https://app.netlify.com/sites/testitori/deploys/67440f9d37e17c000876ba55
😎 Deploy Preview	https://deploy-preview-1399--testitori.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

n0izn0iz

surely there must be a way to compute a zoom level that always fit the width of the map no?

it should probably be used in #1398 also

packages/utils/feed/map.ts

…leaflet

n0izn0iz

tested and reviewed

hthieu1110

Could you take a look at my comments pls :) thanks.

hthieu1110 · 2024-11-23T15:14:14Z

packages/components/socialFeed/Map/Map.web.tsx

+    const lat = 45;
+    const lng = 90;
+
+    const adjustedWorldMapBounds: LatLngBoundsLiteral = [
+      [-lat, -lng],
+      [lat, lng],
+    ];


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

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

fixed here: b42b56d

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.

const in js don't exist

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

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.

we can vote on this

hthieu1110 · 2024-11-23T15:17:03Z

packages/components/socialFeed/Map/Map.web.tsx

+      [lat, lng],
+    ];
+
+    const updateMinZoom = () => {


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])

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

you can extract a hook though if you really want

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

fixed here: 066f8cc

…onent

clegirar · 2024-12-02T13:39:24Z

I read comments from both PR's (#1398 #1399) and it refers to each other, can we just have one PR with split and specific commits for more clarity ?

For the code it's seems okay for me, i understand norman who said that it should be better to have a calcul that fit with any screen size, and it's not really what it's implemented here, can you explain @sujal-into where MAP_HALF_BOUND values came from ?

But i tested the behavior and i tried to resize in big and small screens and it works well 👍

clegirar · 2024-12-02T22:48:49Z

This PR will receive some changes from this PR #1398
I'll explain all changes that i'm going to bring 👍

Signed-off-by: clegirar <[email protected]>

clegirar · 2024-12-03T11:23:31Z

MAP_MAX_BOUND variable is the GPS coordinates (latitude, longitude) of the south-west corner and the north-east corner of a classic planisphere (so they don't come from nowhere).
zoomSnap is the value that forces the map's zoom level to always be a multiple of this, so we decreased it to have a better fit with our container.
And the last thing I want to mentioned is that the initial leaflet map is a square and our container is a rectangle, so this is the main reason we have so much difficulty to fit perfectly with our container, it's because we can't perfectly fit squares into rectangle without have some white strips on the sides.

We are obligate to pass zoom initial value to MapContainer so it must be arbitrary, we set it to 2.15 (default value that fit with the rectangle in many cases, without white strips on the sides).
I moved updateMinZoom directly in MapManager because it doesn't make a lot of sense to extract a hook that is usable in just one case, and can be write in 6 lines .
This hook initially did more than 6 lines but the event listener didn't work, so I removed it and I don't understand why we have to handle a resize window behaviour ? I understand we have to handle some responsiveness but resizing windows is not included in that part imo.

One thing about the ternary condition in zoom props, but the consultedPostLocation seems to be not correctly works with MapContainer, but I found a props in setView for consulted and creation post condition, it's that setView take a zoom props that we can directly pass to it, found it more appropriate, and it works well.

Found it little bit confusing to have one value minZoom that is shared for zoom and minZoom value in MapContainer, but we don't need to follow the zoom value for the moment so I don't have opinions on that.

netlify · 2024-12-03T11:26:42Z

✅ Deploy Preview for gno-dapp ready!

Name	Link
🔨 Latest commit	`49d4069`
🔍 Latest deploy log	https://app.netlify.com/sites/gno-dapp/deploys/6751a503ebea8d0008be1d52
😎 Deploy Preview	https://deploy-preview-1399--gno-dapp.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

n0izn0iz · 2024-12-05T12:46:35Z

aussi, en vertu de l'article 49 alinéa 3 de la constitution, j'engage la responsabilité de mon organisation décentralisée sur l'application de la requête de fusionnement numéro 1399

n0izn0iz

would prefer that it's fully responsive but it's already pretty good

clegirar · 2024-12-05T12:56:35Z

Like even set the boundaries dynamically ?

n0izn0iz · 2024-12-05T13:33:03Z

I mean add back the resize event handler, but it's okay to do it later

n0izn0iz · 2024-12-06T07:36:05Z

qu'ils viennent me chercher

fix: max bound set for whole map

3448435

sujal-into self-assigned this Nov 19, 2024

sujal-into added the Social Feed Everything related to the Social Feed label Nov 19, 2024

sujal-into requested review from WaDadidou and n0izn0iz November 19, 2024 10:56

n0izn0iz reviewed Nov 19, 2024

View reviewed changes

WaDadidou requested changes Nov 19, 2024

View reviewed changes

packages/utils/feed/map.ts Outdated Show resolved Hide resolved

packages/utils/feed/map.ts Outdated Show resolved Hide resolved

sujal-into added 2 commits November 20, 2024 10:03

removed the latLng BooundExpression type

3529fa4

Replaced the latlngBoundExpression type with LatlngBoundLietral from …

773a6c6

…leaflet

sujal-into mentioned this pull request Nov 20, 2024

fix(feed): setting zoom level when location not provided #1398

Closed

fix: responsive zoom level

b63bc03

sujal-into requested review from WaDadidou and n0izn0iz November 20, 2024 09:32

n0izn0iz previously approved these changes Nov 20, 2024

View reviewed changes

WaDadidou changed the title ~~fix: max bound set for whole map~~ fix(feed): max bound set for whole map Nov 21, 2024

hthieu1110 requested changes Nov 23, 2024

View reviewed changes

sujal-into added 2 commits November 25, 2024 11:25

fix: added the map half bound

b42b56d

fix: extraced the useUpdateMinZoom hooks from the DynamicMinZoom comp…

066f8cc

…onent

sujal-into dismissed n0izn0iz’s stale review via 066f8cc November 25, 2024 05:40

lint-fix: removed the unused imports

8835c32

sujal-into requested review from n0izn0iz and hthieu1110 November 25, 2024 05:49

fix: zoom and boundaries for web Map Feed

4c743ac

Signed-off-by: clegirar <[email protected]>

Merge branch 'main' into bugfix/zoom-out-bounds

7048c47

clegirar changed the title ~~fix(feed): max bound set for whole map~~ fix(feed): boundaries and zoom for map feed Dec 5, 2024

n0izn0iz approved these changes Dec 5, 2024

View reviewed changes

Merge branch 'main' into bugfix/zoom-out-bounds

49d4069

n0izn0iz merged commit fb82523 into main Dec 6, 2024
23 checks passed

n0izn0iz deleted the bugfix/zoom-out-bounds branch December 6, 2024 07:36

n0izn0iz mentioned this pull request Dec 8, 2024

bug(feed): dezoomed map by default instead of Paris #1367

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sujal-into commented Nov 19, 2024

netlify bot commented Nov 19, 2024 •

edited

Loading

netlify bot commented Nov 19, 2024 •

edited

Loading

n0izn0iz left a comment •

edited

Loading

n0izn0iz left a comment

hthieu1110 left a comment

hthieu1110 Nov 23, 2024

n0izn0iz Nov 23, 2024

sujal-into Nov 25, 2024

hthieu1110 Dec 4, 2024

n0izn0iz Dec 4, 2024

n0izn0iz Dec 4, 2024

n0izn0iz Dec 4, 2024 •

edited

Loading

n0izn0iz Dec 4, 2024

hthieu1110 Nov 23, 2024

n0izn0iz Nov 23, 2024 •

edited

Loading

n0izn0iz Nov 23, 2024

sujal-into Nov 25, 2024

clegirar commented Dec 2, 2024

clegirar commented Dec 2, 2024

clegirar commented Dec 3, 2024 •

edited

Loading

netlify bot commented Dec 3, 2024 •

edited

Loading

n0izn0iz commented Dec 5, 2024 •

edited

Loading

n0izn0iz left a comment

clegirar commented Dec 5, 2024

n0izn0iz commented Dec 5, 2024

n0izn0iz commented Dec 6, 2024

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

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

Conversation

sujal-into commented Nov 19, 2024

netlify bot commented Nov 19, 2024 • edited Loading

✅ Deploy Preview for teritori-dapp ready!

netlify bot commented Nov 19, 2024 • edited Loading

✅ Deploy Preview for testitori ready!

n0izn0iz left a comment • edited Loading

Choose a reason for hiding this comment

n0izn0iz left a comment

Choose a reason for hiding this comment

hthieu1110 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n0izn0iz Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n0izn0iz Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clegirar commented Dec 2, 2024

clegirar commented Dec 2, 2024

clegirar commented Dec 3, 2024 • edited Loading

netlify bot commented Dec 3, 2024 • edited Loading

✅ Deploy Preview for gno-dapp ready!

n0izn0iz commented Dec 5, 2024 • edited Loading

n0izn0iz left a comment

Choose a reason for hiding this comment

clegirar commented Dec 5, 2024

n0izn0iz commented Dec 5, 2024

n0izn0iz commented Dec 6, 2024

netlify bot commented Nov 19, 2024 •

edited

Loading

netlify bot commented Nov 19, 2024 •

edited

Loading

n0izn0iz left a comment •

edited

Loading

n0izn0iz Dec 4, 2024 •

edited

Loading

n0izn0iz Nov 23, 2024 •

edited

Loading

clegirar commented Dec 3, 2024 •

edited

Loading

netlify bot commented Dec 3, 2024 •

edited

Loading

n0izn0iz commented Dec 5, 2024 •

edited

Loading