Feat/google maps link #10003
Feat/google maps link #10003
Conversation
WalkthroughThe pull request introduces enhancements to the facility pages by adding a Google Maps link feature. It includes a new localization entry for "Show On Maps" and modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@rithviknishad Kindly review this. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/types/facility/facility.ts (1)
31-32: Consider using number type for geographical coordinates.The latitude and longitude properties are currently typed as string, but they represent numerical values. Consider:
- Using
numbertype instead ofstringfor better type safety and to avoid unnecessary string-to-number conversions.- Adding validation constraints using JSDoc or custom type guards to ensure valid coordinate ranges:
- Latitude: -90 to 90
- Longitude: -180 to 180
- latitude: string; - longitude: string; + /** Latitude in decimal degrees, range: -90 to 90 */ + latitude: number; + /** Longitude in decimal degrees, range: -180 to 180 */ + longitude: number;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
public/locale/en.json(1 hunks)public/locale/hi.json(1 hunks)public/locale/kn.json(1 hunks)public/locale/ml.json(1 hunks)public/locale/mr.json(1 hunks)public/locale/ta.json(1 hunks)src/components/Facility/FacilityHome.tsx(3 hunks)src/types/facility/facility.ts(1 hunks)
🔇 Additional comments (8)
src/components/Facility/FacilityHome.tsx (2)
287-297: LGTM! Well-implemented Google Maps link.The implementation:
- Correctly checks for both latitude and longitude before rendering
- Uses proper security attributes (target="_blank" with rel="noreferrer")
- Follows accessibility best practices with proper text and icon
281-281: LGTM! Margin adjustment.The margin adjustment from mt-2 to mt-1 improves the visual spacing.
public/locale/mr.json (1)
42-42: LGTM! Correct Marathi translation.The translation "Google नकाशे" is appropriate and consistent with Marathi language conventions.
public/locale/hi.json (1)
395-395: LGTM! Correct Hindi translation.The translation "Google मैप्स" is appropriate and consistent with Hindi language conventions.
public/locale/kn.json (1)
397-397: Translation looks good!The Kannada translation "Google ನಕ್ಷೆಗಳು" is accurate and follows proper localization practices.
public/locale/ta.json (1)
396-396: Translation looks good!The Tamil translation "Google வரைபடங்கள்" is accurate and follows proper localization practices.
public/locale/ml.json (1)
998-998: LGTM! Translation looks good.The Malayalam translation for "Google Maps" is accurate and consistent with the translations added in other locale files.
public/locale/en.json (1)
1081-1081: LGTM! The translation key follows the established conventions.The addition of the "google_maps" translation key with value "Google Maps" is consistent with the existing naming patterns and properly handles the capitalization of the proper noun.
|
@rahulharpal1603 Do not add translations for all languages except en.json |
Thanks, I will fix it |
…le log in FacilityHome component
rithviknishad
added
needs testing
needs review
and removed
question
|
LGTM |
I thought from the perspective of the district admin, where he/she can see details about various facilities. In the facilities page for the admin, many facilities were listed instead of only one. |
|
I think it should be part of the public page where users can use the maps to navigate to the location, don't think its required in the internal facility side. (Since we have added it, I maynot mind keeping it) Also Care is a DPG, lets not hardode |
bodhish
removed
question
|
@rithviknishad I have added the conditional link rendering, based on whether the device is android or not. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Facility/FacilityHome.tsx (1)
126-148: Consider environment-based configuration for maps URL.The Google Maps URL should be configurable through environment variables to support different map providers or configurations.
Consider this approach:
+const MAPS_URL = process.env.MAPS_URL || 'https://www.google.com/maps/search/'; + const getMapsLink = (latitude: number, longitude: number) => { return isAndroidDevice ? ( <a className="text-sm text-primary flex items-center gap-1 w-max" href={`geo:0,0?q=${latitude},${longitude}`} target="_blank" rel="noreferrer" > {t("show_on_maps")} <SquareArrowOutUpRight className="h-3 w-3" /> </a> ) : ( <a className="text-sm text-primary flex items-center gap-1 w-max" - href={`https://www.google.com/maps/search/?api=1&query=${latitude},${longitude}`} + href={`${MAPS_URL}?api=1&query=${latitude},${longitude}`} target="_blank" rel="noreferrer" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/Utils/utils.ts(1 hunks)src/components/Facility/FacilityHome.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🔇 Additional comments (2)
src/components/Facility/FacilityHome.tsx (2)
98-98: Remove debug console.log statement.Remove the console.log statement used for debugging.
- console.log(navigator.platform);
336-341: LGTM! Proper null checks for coordinates.The implementation correctly checks for both latitude and longitude before rendering the maps link.
rithviknishad
added
needs testing
needs review
and removed
changes required
Deploy-Failed
Was this done? |
rithviknishad
added
question
I will add that today. |
…feat/Google-Maps-Link
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Facility/FacilityHome.tsx (1)
354-359: Add null check for coordinates.While there's a conditional check for the existence of coordinates, it's good practice to validate that they are valid numbers.
- {facilityData.latitude && - facilityData.longitude && + {typeof facilityData.latitude === 'number' && + typeof facilityData.longitude === 'number' && getMapsLink( facilityData.latitude, facilityData.longitude, )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/Utils/utils.ts(1 hunks)src/components/Facility/FacilityHome.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locale/en.json
- src/Utils/utils.ts
🔇 Additional comments (2)
src/components/Facility/FacilityHome.tsx (2)
9-15: LGTM! Clean import organization.The imports are well-organized and the new
SquareArrowOutUpRighticon is appropriately added to the existing lucide-react imports.
104-104: Remove debug console.log statement.The console.log statement for
navigator.platformappears to be used for debugging and should be removed before merging.
…dmin and public pages.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Facility/FacilityHome.tsx(3 hunks)src/components/Facility/FacilityMapsLink.tsx(1 hunks)src/pages/Facility/FacilityDetailsPage.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/FacilityHome.tsx
🔇 Additional comments (2)
src/components/Facility/FacilityMapsLink.tsx (1)
18-26: LGTM! Accessibility attributes are properly set.The link implementation follows accessibility best practices with proper attributes for external links.
src/pages/Facility/FacilityDetailsPage.tsx (1)
118-123: LGTM! Proper null checks for coordinates.The component is correctly rendered only when both latitude and longitude are present.
| const href = isAndroidDevice | ||
| ? `geo:0,0?q=${latitude},${longitude}` | ||
| : `https://maps.google.com/?q=${latitude},${longitude}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider environment-based URL configuration.
Based on PR comments from bodhish, the maps URL should be configurable via environment variables rather than hardcoded.
+import { getConfig } from "@/config";
+
export const FacilityMapsLink = ({
latitude,
longitude,
}: {
latitude: number;
longitude: number;
}) => {
const { t } = useTranslation();
+ const mapsUrl = getConfig("MAPS_URL");
+ if (!mapsUrl) return null;
+
const href = isAndroidDevice
- ? `geo:0,0?q=${latitude},${longitude}`
- : `https://maps.google.com/?q=${latitude},${longitude}`;
+ ? `geo:0,0?q=${latitude},${longitude}`
+ : `${mapsUrl}?q=${latitude},${longitude}`;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityMapsLink.tsx(1 hunks)
🔇 Additional comments (3)
src/components/Facility/FacilityMapsLink.tsx (3)
1-4: LGTM!The imports are well-organized and follow project conventions.
20-22: Make maps URL configurable via environment.The hardcoded Google Maps URL should be configurable via environment variables.
Based on the PR comments discussion about device compatibility:
- Consider adding a fallback mechanism for Android devices where the geo URI scheme isn't supported.
- Add error tracking to monitor if users encounter issues with the geo URI scheme.
23-33: LGTM!The component follows accessibility best practices with:
- Appropriate security attributes for external links (
rel="noreferrer")- Clear visual indication of external link (icon)
- Internationalization support
|
@rithviknishad I have added the maps link to the public pages also. Please review. |
bodhish
added
needs testing
needs review
and removed
question
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Facility/FacilityMapsLink.tsx (1)
30-31: Enhance link accessibility and security.The link's accessibility and security can be improved:
- Add an
aria-labelfor screen readers- Add descriptive text for external link behavior
- Simplify the target logic
const target = isAndroidDevice ? "_self" : "_blank"; return ( <a className="text-sm text-primary flex items-center gap-1 w-max" href={href} target={target} rel="noreferrer" + aria-label={`Show location at coordinates ${latitude},${longitude} on map`} + {...(!isAndroidDevice && { + 'aria-description': 'Opens in a new tab' + })} >Also applies to: 32-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityMapsLink.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Facility/FacilityMapsLink.tsx (1)
27-29: Configure maps URL via environment variables.The hardcoded Google Maps URL should be configurable.
| const useValidateCoordinates = (latitude: number, longitude: number) => { | ||
| useEffect(() => { | ||
| if (latitude < -90 || latitude > 90) { | ||
| console.error("Invalid latitude. Must be between -90 and 90 degrees."); | ||
| } | ||
| if (longitude < -180 || longitude > 180) { | ||
| console.error("Invalid longitude. Must be between -180 and 180 degrees."); | ||
| } | ||
| }, [latitude, longitude]); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance coordinate validation.
The current validation approach using console.error in a hook has several limitations:
- Console messages aren't suitable for production environments
- Validation doesn't prevent invalid coordinates from being used
- Validation logic isn't reusable outside React components
Consider this alternative approach:
// utils/coordinates.ts
export class InvalidCoordinateError extends Error {
constructor(message: string) {
super(message);
this.name = 'InvalidCoordinateError';
}
}
export const validateCoordinates = (latitude: number, longitude: number) => {
if (latitude < -90 || latitude > 90) {
throw new InvalidCoordinateError("Invalid latitude. Must be between -90 and 90 degrees.");
}
if (longitude < -180 || longitude > 180) {
throw new InvalidCoordinateError("Invalid longitude. Must be between -180 and 180 degrees.");
}
return { latitude, longitude };
};
// FacilityMapsLink.tsx
const useValidateCoordinates = (latitude: number, longitude: number) => {
useEffect(() => {
try {
validateCoordinates(latitude, longitude);
} catch (error) {
// Log to error monitoring service
console.error(error);
}
}, [latitude, longitude]);
};
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Demo Video:
2025-01-16.02-17-42.mp4
Merge Checklist
Summary by CodeRabbit
New Features
Style
Bug Fixes