Modernize farm selection and sidebar navigation UI#457
Conversation
…r feature breakdowns for new users, and a dedicated Atlas section for existing farms. Improved sidebar navigation by adding "muted" states for unavailable features and clarifying the distinction between the farm list and farm overview
🦋 Changeset detectedLatest commit: d3b8248 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #457 +/- ##
============================================
Coverage 88.04% 88.04%
============================================
Files 91 91
Lines 4625 4625
Branches 1494 1494
============================================
Hits 4072 4072
Misses 553 553
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughModernizes the farm selection screen and sidebar: introduces tooltip-wrapped disabled states, collapsible Atlas/Balans sections with nested sub-links, state-driven rendering based on farm selection and wizard state, and redesigned farm cards with role badges and Atlas cards. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
changed the title
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-app/app/components/blocks/sidebar/farm.tsx (1)
96-101:⚠️ Potential issue | 🟠 Major
fertilizersLinkis not gated byisCreateFarmWizard, unlikefieldsLinkandrotationLink.During the create-farm wizard,
farmIdcan be set (the wizard creates a farm ID early).fieldsLink(line 79) androtationLink(line 88) correctly returnundefinedwhenisCreateFarmWizardis true, disabling those sidebar items.fertilizersLinkskips this check, so the Meststoffen item remains clickable during the wizard — likely unintended.🐛 Suggested fix
let fertilizersLink: string | undefined - if (farmId && farmId !== "undefined") { + if (isCreateFarmWizard) { + fertilizersLink = undefined + } else if (farmId && farmId !== "undefined") { fertilizersLink = `/farm/${farmId}/fertilizers` } else { fertilizersLink = undefined }
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 153-155: Update the hardcoded title text inside the CardTitle
element (the JSX using the CardTitle component) where it currently reads
"Bedrijf aamaken" to correct the typo to "Bedrijf aanmaken"; locate the
CardTitle JSX in the farm._index route component and change the string content
accordingly.
- Around line 186-191: Fix the typo in the JSX label: change the bold text
"Gebruikruimte:" to "Gebruiksruimte:" inside the span in the farm index route
component (the JSX block that renders <span><b>Gebruikruimte:</b> ...</span>) so
it matches the sidebar label and other usages.
- Around line 304-323: The role label mapping in the JSX that iterates
farm.roles (the map rendering producing Badge elements) is missing an explicit
case for "researcher" and thus shows "Lid" instead of "Onderzoeker", causing
inconsistency with the mapping used in the Farm sidebar (the mapping around
getRoleLabel in farm.tsx); fix by extracting a shared helper (e.g.,
getRoleLabel(role: string) or a ROLE_LABELS constant) and replace the inline
ternary inside the Badge rendering with a call to that helper so "owner" →
"Eigenaar", "advisor" → "Adviseur", "researcher" → "Onderzoeker" and a default
fallback remain consistent across components.
- Line 8: The import named Map from lucide-react shadows the global Map
constructor; rename the imported symbol (e.g., to MapIcon or LucideMap) in the
import statement and update all usages of Map in this module (notably the icon
usage around the previously referenced usage at line ~402) to the new name so
the global Map is no longer shadowed.
- Around line 92-111: The SupportNote component builds an unreliable support
address using window.location.hostname (SupportNote), which yields invalid
emails in dev/staging; update the logic to read a configured supportEmail from
clientConfig (exposed via an environment variable) with a fallback to the
existing dynamic `support@${window.location.hostname}` behavior, and use that
value when constructing the mailto link (replace the inline supportEmail
construction in SupportNote with clientConfig.supportEmail ||
`support@${window.location.hostname}`), ensuring clientConfig is loaded where
SupportNote is rendered.
🧹 Nitpick comments (5)
fdm-app/app/components/blocks/sidebar/apps.tsx (2)
129-138: Disabled Atlas lacks a tooltip, unlike the other disabled items.Balans (line 209), Bemestingsadvies (line 279), and Gebruiksruimte (line 312) all wrap their disabled states in a
<Tooltip>explaining why the item is unavailable. The disabled Atlas button is the only one without this tooltip, creating an inconsistent UX — a user in the create-farm wizard gets no hint about why Atlas is greyed out.♻️ Suggested fix: wrap in Tooltip like the other disabled items
) : ( + <Tooltip> + <TooltipTrigger asChild> <SidebarMenuButton isActive={false} className="hover:bg-transparent hover:text-muted-foreground active:bg-transparent active:text-muted-foreground opacity-50 cursor-not-allowed" > <MapIcon className="text-muted-foreground" /> <span className="text-muted-foreground"> Atlas </span> </SidebarMenuButton> + </TooltipTrigger> + <TooltipContent side="right"> + Atlas is niet beschikbaar tijdens het aanmaken van een bedrijf + </TooltipContent> + </Tooltip> )}
106-334: The disabled-state className string is duplicated four times.The class string
"hover:bg-transparent hover:text-muted-foreground active:bg-transparent active:text-muted-foreground opacity-50 cursor-not-allowed"is repeated at lines 132, 213, 283, and 316. Consider extracting it into a constant (e.g.,const disabledMenuButtonClassName = "...") at the top of the component to reduce duplication and make future styling tweaks easier. The same pattern also appears infarm.tsx(lines 249, 282, 315, 348).fdm-app/app/routes/farm._index.tsx (2)
389-392: Minor text issue: "gegevens" should likely be singular or rephrased.Line 391: "Raadpleeg openbare kaarten en ruimtelijke gegevens." — In Dutch, "gegevens" is already the standard plural form. However, it looks like there might be a trailing 's' issue making it read oddly. Consider "ruimtelijke gegevens" or simply "ruimtelijke data" for clarity. This is a very minor nit.
284-387: Farm cards and Atlas cards share a nearly identical card pattern — consider extracting a reusable card component.The card structure (icon + title header, content area, footer with arrow) is repeated across farm cards (lines 286–367), the "new farm" card (lines 370–386), and the three Atlas cards (lines 394–479). A small abstraction (e.g.,
NavigationCard) could reduce duplication and make future styling changes easier.fdm-app/app/components/blocks/sidebar/farm.tsx (1)
197-201: Calendar URL rewrite regex could match unintended path segments.The regex
/\/(\d{4}|all)/replaces the first path segment that looks like a 4-digit number or"all". While farm IDs are UUIDs (safe today), this could silently break if any other 4-digit segment appears in the URL in the future (e.g., a port-like path component). A more targeted regex anchored to the expected position would be more robust, e.g.:const newUrl = location.pathname.replace( /\/farm\/[^/]+\/(\d{4}|all)/, `/farm/${farmId}/${item}`, )This is a minor robustness concern for now.
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
nice improvement from user perspective. LGTM.
2 participants
Summary by CodeRabbit
New Features
UI/UX Improvements