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
✨ Finishing Touches
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
|
instead of specifying a url in env, we could do: https://en.wikipedia.org/wiki/Geo_URI_scheme which should ideally open the Open in Maps thing where user can decide their preferred app. |
Okay, I will make the changes. |
…feat/Google-Maps-Link
Hi, I was testing this on different devices and found that these types of URLs are only compatible with Android devices. On Windows, Mac, Linux and IOS, this doesn't work. I think we would have to go with the google maps link. |
…ilityData interface
|
👋 Hi, @rahulharpal1603, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
We can check the device and do accordingly |
Sure, I can do that. |
…feat/Google-Maps-Link
|
@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.
| /** | ||
| * Referred from: https://stackoverflow.com/questions/6031412/detect-android-phone-via-javascript-jquery | ||
| * @returns `true` if device is Android, else `false` | ||
| */ | ||
| function _isAndroidDevice(): boolean { | ||
| return /android/i.test(navigator.userAgent); | ||
| } | ||
|
|
||
| export const isAndroidDevice = _isAndroidDevice(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using more robust device detection methods.
The current implementation has several potential issues:
- User agent sniffing is unreliable as it can be spoofed.
- The constant is computed at module load time, which might not work in SSR environments.
Consider these alternatives:
- Use feature detection:
-function _isAndroidDevice(): boolean {
- return /android/i.test(navigator.userAgent);
-}
-
-export const isAndroidDevice = _isAndroidDevice();
+export function isAndroidDevice(): boolean {
+ if (typeof window === 'undefined') return false;
+
+ // Check if the platform-specific features are available
+ return (
+ 'onorientationchange' in window &&
+ 'ontouchstart' in window &&
+ navigator.maxTouchPoints > 0 &&
+ !('standalone' in navigator)
+ );
+}- Or use modern APIs:
export function isAndroidDevice(): boolean {
if (typeof window === 'undefined') return false;
return navigator.userAgentData?.platform === 'Android';
}
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