Repository navigation
feat(Map): add entrance interest filtering and separate map controls - #1547
Conversation
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Reviewed the diff against the live API, the existing rating surfaces (InterestSummary, CommentRatings), and node probes of utils/interest.js. The split into DataDisplayControl + FiltersControl is the right cut and controlStyles.js keeps them reading as siblings. Notes below, heaviest first.
Worth a decision
1. The map rounds stars; the entrance page doesn't — so the two surfaces label the same cave differently.
InterestSummary renders the average with precision={0.1} and prints {avg} / 5 to one decimal. The new getInterestLevel rounds to the nearest whole star. Two adjacent API values that the entrance page presents identically diverge on the map:
aestheticism |
Entrance page | Map popup | Passes 3★ min? |
|---|---|---|---|
4.9 |
2.5 / 5 |
★★ "Limited interest" | no |
5.0 |
2.5 / 5 |
★★★ "Interesting" | yes |
The PR's stated goal — the threshold agreeing with the stars shown — holds within the map, but it's bought by disagreeing with the page the user reaches by clicking through the popup. CommentRatings also interpolates (getRatingLevelIds(value / 2) returns a two-label range for half-stars) rather than rounding, so the label text diverges too.
Is that the intended trade? If yes, Filter by interest help is the natural place to say the map rounds to whole stars — it already does the useful work of disclosing that unrated entrances get hidden.
2. The "filters active" dot shows while the filters are inert.
FiltersControl.jsx — {hasActiveFilters && <ActiveFiltersDot />} isn't gated on filtersDisabled. Zoomed out, or with the entrance layer off, the panel greys itself out and says "Zoom in to enable filters", but the toggle still wears the warning dot. A user who zooms out watches entrances reappear while the badge insists a filter is applied.
Either gate it on !filtersDisabled, or keep it and redefine it as "you have saved filters" — in which case the greyed panel should say that the saved filters aren't currently being applied. Related: the Reset button sits outside the pointer-events: none wrapper, so it stays clickable while everything above it is disabled. Probably deliberate (it's the only way to clear the dot from there) but worth confirming it reads that way.
3. InterestStars has no accessible name — unlike the badge its header comment cites as the model.
DataQualityBadge carries an SVG <title> with a localized, value-bearing name. InterestStars emits five bare ★ glyphs where filled and empty use the same glyph and differ only in colour, so a screen reader announces five stars for a two-star rating — actively wrong rather than merely absent.
In the popup it's the icon of <Information> and the sibling value carries the meaning, so the cheap correct fix is aria-hidden="true" on the wrapper span. To make it self-describing instead: role="img" plus an aria-label off the {rating} out of 5 stars key CommentRatings already uses.
4. Star⇄scale conversion now exists twice, with different rules.
utils/interest.js has getInterestLevel = min(5, max(1, round(v / 2))). FiltersControl.jsx has private interestToStars = v => Math.round(v / 2) — no clamp — plus starsToInterest. They agree today only because minInterest is always an exact multiple of 2.
But minInterest comes from useLocalStorage('grottocenter_minInterest', …) with no validation on read, and the un-clamped path is the one a stray value hits: a stored 12 yields Rating value={6} max={5}, and meetsMinimumInterest(v, 12) requires 6 stars, so every entrance disappears with nothing on screen explaining why (Reset does recover it). The key is new in this PR so there's no legacy data to worry about — the point is that the two converters should live together in utils/interest.js, with the clamp applied once, so they can't drift.
Smaller
5. The new outline constant is applied at two widths, and its own comment argues against one of them.
MAP_MARKER_OUTLINE_WIDTH = 1.5 is used both as an SVG stroke-width (with vector-effect="non-scaling-stroke" — correct, 1.5 device px) and as a plain CSS border width: MeasureControl 2 → 1.5, UserLocationMarker 2.5 → 1.5, WaypointOffscreenIndicator 2 → 1.5. Meanwhile ClusterLayer adds CSS_BUBBLE_OUTLINE_WIDTH = 2 justified by "CSS borders read slightly thinner than the network's SVG stroke at 1.5px" — the PR argues 1.5 is too thin for CSS, then uses 1.5 for CSS in three places. Worth one story either way.
Also: 1.5px CSS borders antialias on 1× displays where 2px was crisp, and the user-location dot is a 40% reduction on the most prominent marker on the map. That's the thing to eyeball for the visual-review box that's still unchecked.
6. ClusterLayer.jsx — comment says 1.7px, code produces 2px.
"a border compensated for the scale so its visible width remains 1.7 CSS pixels" — CSS_BUBBLE_OUTLINE_WIDTH / 0.88 is 2.27px, which after scale(0.88) renders at 2px. The hover rule's / 0.95 is right; just the number in the prose.
7. The rounding rule is the feature, and its boundaries are untested.
interest.test.js has two cases and never calls getInterestLevel or getInterestLabelKey. Load-bearing and uncovered:
- The
max(1, …)floor.getInterestLevel(0.1)is 1, so a 1★ minimum excludes nothing except unrated entrances — I confirmed every non-null value in 0.1–10 passesmin=2. That's a deliberate and genuinely surprising semantic; pin it with a test so it isn't "fixed" later by accident. - The
min(5, …)clamp. - The whole-star boundaries 3 / 5 / 7 / 9, where the filter actually flips.
nullvs0. The floor's justification ("a comment giving 0.5/10 is still one star's worth") holds only because the API never sends 0 — swagger onaestheticism: "Ratings of0and unrated comments are excluded, andnullis returned when no comment rates the entrance." Worth asserting thenullcontract so the floor stays correct if that ever changes.
utils/interest.js has a clean import graph, so this is the cheap tier. MapMassif.property.test.jsx shows fast-check is already in play here — a round-trip over starsToInterest/interestToStars and a monotonicity property on meetsMinimumInterest (raising the minimum never adds entrances) would be a few lines and would cover the whole table.
8. Dead re-export. FiltersControl.jsx:395 — export { DEFAULT_MIN_INTEREST };. Nothing imports it from there; index.jsx takes it from ./constants. The re-export is also the only reason the import on line 17 isn't flagged as unused. Drop both.
9. entranceMarkers PropTypes. markerType in index.jsx still lists only latitude / longitude / id / name, but the filter now reads e.aestheticism. EntrancePopup got the field added to its shape; the map's didn't.
10. Disabled-reason priority. disabledReasonKey prefers "Turn on entrances" over "Zoom in". With entrances off and zoomed out, following that advice still leaves the filters disabled. Zoom is the more common blocker, so the other order may guide better.
Verified, not just assumed
- All 12 new keys are present in all 15 language files.
MAP_TOUR_VERSION→ 2 paired with the cypressmapTourSeen_v2update. Easy to forget, and it would have silently broken every e2e spec.vector-effect="non-scaling-stroke"on the network/organization markers is safe, not just plausible: a 106.667 viewBox rendered at 38px made the oldstrokeWidth="4"draw ~1.43 device px, so 1.5 non-scaling lands within a rounding error of the current appearance.LocationControlalways mounts inMapClusters(isLocationControlAlways) and has no early return, so the new tour step's selector can't miss.- The
InterestStars"no MUI/emotion in popups" justification is real —useRenderPopupserializes throughrenderToString, andInformation's own comment documents that emotion's hashed classes aren't injected there. MUIRatingwould render unstyled, so the hand-rolled stars are the right call rather than a missed reuse.
|
Addressed the review in
Checks: full ESLint passed with zero errors; all 26 interest tests passed; translations are synchronized and sorted. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Re-reviewed at a90f30a9. All ten points are addressed, and the first one was resolved better than what I suggested — instead of documenting the rounding, you moved the map to half-star steps, so 4.9 and 5.0 now both read 2.5 stars and the popup label goes through getRatingLevelIds(value / 2, INTEREST_LEVELS), which is exactly the convention CommentRatings already uses. That removes the map/entrance-page divergence rather than explaining it.
What I verified independently rather than taking on trust:
- Map and entrance page now agree across the whole reachable domain. Compared
getInterestLevel(v)againstInterestSummary'sv / 2for every boundary value; the largest gap is 0.25, i.e. within the half-star step, everywhere an average of 1–10 can land. - All 26 expectations in
interest.test.jsreproduce against a standalone import of the module, independent of the assertions in the file. - Garbage stored minima are genuinely neutralized.
'abc',undefined,-3,Infinityall normalize to0throughstarsToInterest(interestToStars(...)), so the silent empty map from point 4 is fixed at the source rather than at the render site. - Translations are complete and sorted. All 12 new keys present in all 15 language files;
scripts/translations/sort.js --checkexits 0.
Three things left, none blocking.
The monotonicity property never exercises a half-star minimum
packages/web-app/src/utils/interest.test.js:80 feeds fc.integer({ min: 0, max: 10 }) into starsToInterest, but that function's domain is stars (0–5), not the 0–10 interest scale. Everything from 5 upward clamps to 10, so the 11 generated inputs collapse to 6 distinct minima — and not one of them is a half star, despite half-star minima being the whole point of this revision.
The round-trip test directly above it gets this right with halfStars / 2. Same shape works here:
fc.integer({ min: 0, max: 10 }).map(halfStars => halfStars / 2)testing.md frames the check as "could a wrong implementation pass this arbitrary?" — as written, an implementation that mishandled 2.5 would pass.
The interest row is the only popup row with no accessible name
InterestStars/index.jsx:13 sets aria-hidden="true", so a screen reader gets the joined level label with nothing identifying it as a rating — the other rows all carry a field name. Its sibling DataQualityBadge solves this with an SVG <title> (Data quality score: {value}/100), and the {rating} out of 5 stars key already exists in en.json. role="img" plus an aria-label off that key is roughly the same edit you already made, and matches the precedent.
EntrancePopup guards differently from the module it calls
EntrancePopup.jsx:57 gates on entrance.aestheticism > 0 while interest.js gates on Number.isFinite. For a non-numeric truthy value the row renders with no icon and an empty value instead of not rendering. getInterestLevel(entrance.aestheticism) != null makes the module the single source of truth for what counts as rated.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the three remaining points from ClemRz's re-review in
Added focused tests for the popup's valid and invalid values and the half-star image label. Targeted unit tests (29 passed) and full ESLint passed. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
urien
left a comment
urien
left a comment
There was a problem hiding this comment.
Effet waouh garanti !
Bravo
|
J'espère que cela motivera les gens à publier des commentaires ! 
Je sais que les canyonneurs utilisent beaucoup cette donnée pour savoir si ça vaut le coup d'aller faire un canyon ou non. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Re-reviewed at 5761cc18. All three items from the last round are fixed, and I checked each by running it rather than by reading the diff.
- The property now exercises half-star minima.
interest.test.js:84's.map(halfStars => halfStars / 2)produces 11 distinct minima throughstarsToInterest(0,1,…,10), including all five odd ones the previous version could never reach. Brute-forced the invariant over value0–10in steps of0.01against all 66 ordered star pairs: 66,066 cases, 0 violations. - The accessible name is right.
role="img"plusaria-labeloff the existing{rating} out of 5 starskey, which is present in all 15 language files.getInterestLevel(4.9)is2.5, so the expected2.5 out of 5 starsholds, androle="img"correctly keeps the ten star glyphs out of the accessible name instead of exposing them alongside the label. - The guard is now single-sourced.
interestLevel != nullis the only gate. ConfirmedInfinity,0and'abc'all returnnullwhile0.1still returns0.5, so the newInfinitytest lands exactly on the case the old> 0check let through.
One caveat on method: I could not execute the two new component tests. This checkout's node_modules predates pdfjs-dist (declared on develop, unrelated to this PR), so vite.config.mjs fails to load. I verified their expectations by hand instead and both should pass — worth a confirming run on your side.
TPST shortens four surfaces, not just the label it was aimed at
Underground time is shared, and fr.json is the only locale changed, so the effect is wider and narrower than the commit message suggests:
InterestSummary.jsx:132— the Property label, the apparent targetEntitiesForm/Comment/index.jsx:139— theInputDurationfield label, which becomesTPST — Heures/TPST — MinutesComment.jsx:177andCommentSnapshots.jsx:70— both feedProperties/Duration, which uses the string as the MUITooltiptitle and the<img alt>
The tooltip case is the one that bites: on an icon, that string is the whole explanation and the image's only accessible name, so collapsing it to an acronym removes what the tooltip was there for. And since the other 14 locales keep the full phrase, if the motivation was the width of the InterestSummary grid cell, English and the rest still show the long label — the layout issue isn't actually solved.
A separate key for the compact label (leaving the tooltip and alt spelled out) covers both, and lets the short form apply to every locale rather than only French.
Worth noting this change is unrelated to the interest filter — InterestSummary.jsx is untouched on develop — so it is also a candidate for its own PR.
|
Addressed the remaining feedback from the three reviews by @ClemRz in
Validation:
|
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Re-reviewed at 20f56260. The split key is right and goes further than I suggested — adding Underground time (short) to all 15 locales rather than only French means the compact label applies everywhere, which was the half of the problem the previous commit left unsolved. Verified: Underground time is back to Temps passé sous terre in fr.json, the short key exists in all 15 files, scripts/translations/sort.js --check exits 0, and the short key is consumed only by InterestSummary.jsx:131 — the comment form label and both Duration call sites keep the full phrase.
Same caveat as last round: this checkout still can't load vite.config.mjs (pdfjs-dist missing from node_modules), so I reasoned about the new test statically rather than running it.
The new test doesn't exercise either thing its name claims
InterestSummary.test.jsx:116 renders a standalone Duration next to the summary, and neither half connects to what it's asserting.
Duration is not in the summary. InterestSummary renders <CustomIcon type="underground_time" />, and CustomIcon/index.jsx:135 sets alt={altTexts[type] ?? type} from a hardcoded English map — no formatMessage anywhere in that path. So beside the French TPST, the icon's accessible name is the English string Underground time, in every locale. That is exactly the "explicit icon beside the compact summary" the commit sets out to guarantee, and it is both untranslated and untested; the separately-rendered Duration belongs to Comment and CommentSnapshots, not here. The shortened label raises the stakes: French users previously had Temps passé sous terre as the label, so the English alt was redundant — now TPST is the only French text and the thing meant to expand it is in English.
The title prop is wired the one way production never wires it. The test passes title="Underground time", the raw key, which does match Duration's contract (formatMessage({ id: title })). But Comment.jsx:177 and CommentSnapshots.jsx:70 both pass formatMessage({ id: 'Underground time' }) — already translated. Running the branch's fr.json through createIntl confirms the second lookup misses and react-intl falls back to returning the id, so the tooltip reads correctly but reports MISSING_TRANSLATION on every render. The test therefore stays green regardless of what happens on the path the app actually takes.
The double-translation is pre-existing and fair to leave for its own PR. But if the intent is to protect the summary's labelling, the assertion wants to be against the summary's own icon — which first needs CustomIcon to translate its alt.
|
Addressed the latest review in
Validation:
|
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Re-reviewed at 30b9d4bb. Both parts of the last finding are properly fixed. The optional alt prop defaulting to altTexts[type] ?? type keeps all 125 other CustomIcon call sites byte-identical, and the test now asserts against the summary's own icon — including the negative queryByRole('img', { name: 'Underground time' }) that pins the English fallback as gone in French, which is the assertion that actually guards the regression. Dropping the standalone Duration render was the right call.
Worth recording why the per-caller prop was the better choice over the tempting shortcut: moving formatMessage inside CustomIcon would look cleaner, but only 19 of the 32 altTexts values are real en.json keys — Bat, Pollution, Rockfall, Touristic and nine others are not — so that version would emit 13 MISSING_TRANSLATION errors. The explicit prop sidesteps that entirely.
(Same caveat as before: pdfjs-dist is still absent from this checkout's node_modules, so I verified the test's expectations by hand rather than running them.)
The time_to_go icon now announces its own label twice
Property renders the icon immediately before the visible label (Property.jsx:66-77), so the approach-time row reads out as Temps d'accès (the alt) followed by Temps d'accès (the label). The underground row doesn't have this problem — its alt is the full phrase and its label is the compact one, so the alt genuinely adds what the label dropped. But Time to go was never shortened, so translating its alt just duplicates the text next to it.
In English this duplication already existed via the hardcoded map, so nothing regressed there; it's the other 14 locales that newly pick it up, since their alt was previously the untranslated Time to go and is now an exact match for the label.
An icon sitting beside its own text label is decorative, so alt="" is the usual treatment:
<CustomIcon type="time_to_go" alt="" />That works as-is with your default parameter — a default only fills in for undefined, so an explicit empty string passes through. Keeping the explicit alt on underground_time and emptying it on time_to_go makes the distinction match the reason the prop was added.
|
Addressed the latest review in
Validation:
|
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1547.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Re-reviewed at f77204b8. This closes my last item, and the test assertion is correct for a non-obvious reason worth stating: the installed aria-query (5.3.2) maps img with alt="" to presentation, and img[alt=""] is the only element selector in that table that maps to presentation — so getByRole('presentation') resolves to exactly one node in that render (the underground icon keeps a non-empty alt and stays img). @testing-library/dom's accessibility-tree exclusion also explicitly leaves presentation and none queryable, so the lookup isn't filtered out. The paired queryByRole('img', { name: "Temps d'accès" }) holds regardless of the readonly Rating also carrying role="img", since the name filter separates them.
I also confirmed the interest logic itself is untouched by the last three commits — utils/interest.js, InterestStars, and everything under components/common/Maps/ are byte-identical to 5761cc18, which is the state I brute-forced the half-star monotonicity and guard behaviour against. So the i18n/alt detour didn't disturb the part this PR is actually about.
Nothing further from me. Standing caveat unchanged: pdfjs-dist is still missing from my checkout's node_modules, so the component tests across these last four commits were verified by reading the resolution rules out of the installed packages rather than by executing them — worth one real yarn test run on the Entry and Maps suites before merge.
feat(Map): add entrance interest filtering and separate map controls
🤔 What
🤷♂️ Why
Map users need to narrow visible entrances by visit interest without mixing filtering options with layer visibility. The interest threshold should agree with the rounded stars shown in the UI.
🔍 How
🔗 Related API PR
🧪 Testing
yarn.cmd lintyarn.cmd buildyarn.cmd translations:check-sortedyarn.cmd test --run src/utils/interest.test.js src/components/common/Maps/MapClusters/OfflineDetailNotice.test.jsx(9 passed)yarn.cmd test --run src/components/appli/EntitiesForm/Guideline/index.test.jsx --testTimeout 15000(4 passed)yarn.cmd test --run --maxWorkers=2 --testTimeout=15000(169 files, 1,173 tests passed). The default 5-second timeout was too short for an unrelated Guideline form test on this machine.To review manually: open
/map, enable entrances, zoom in until markers appear, open the Filters control, select a minimum interest rating, and check the visible entrances and popup stars. Reset the filters, then toggle the entrance layer or zoom out to check the disabled hint.📸 Previews