Add Facility Map Link #10755
Add Facility Map Link #10755
Conversation
WalkthroughThis pull request introduces a new localization entry and a facility map link feature. A new key Changes
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. |
![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 (3)
src/components/Facility/FacilityMapLink.tsx (2)
7-15: Consider extracting validation functions to a shared utility file.The coordinate validation functions could be reused across the application. Consider moving them to a dedicated geolocation utility file.
-const isValidLatitude = (latitude: string) => { - const lat = parseFloat(latitude.trim()); - return Number.isFinite(lat) && lat >= -90 && lat <= 90; -}; - -const isValidLongitude = (longitude: string) => { - const long = parseFloat(longitude.trim()); - return Number.isFinite(long) && long >= -180 && long <= 180; -};Create a new file
src/Utils/geolocation.ts:export const isValidLatitude = (latitude: string) => { const lat = parseFloat(latitude.trim()); return Number.isFinite(lat) && lat >= -90 && lat <= 90; }; export const isValidLongitude = (longitude: string) => { const long = parseFloat(longitude.trim()); return Number.isFinite(long) && long >= -180 && long <= 180; };
17-44:❓ Verification inconclusive
Consider accessibility improvements for the map link.
The link could benefit from additional accessibility attributes:
<Link className="text-primary flex items-center gap-1 w-max" href={href} target={target} rel="noreferrer" + aria-label={`${t("show_on_maps")} for ${t("facility_location")}`} >Also, verify that the link text is properly localized:
🌐 Web query:
What are the WCAG guidelines for opening links in new tabs?💡 Result:
The Web Content Accessibility Guidelines (WCAG) address link behavior through multiple success criteria, particularly regarding predictability and user control when opening new tabs/windows:
Key WCAG Requirements
Success Criterion 3.2 Predictable
Requires web pages to operate predictably[1][4]. Unexpected new tabs/windows (without warning) violate this principle by creating unannounced context changes[1][3][9].Success Criterion 3.2.5 Change on Request (AAA)
Explicitly prohibits automatic new tabs/windows unless:
- Users request them via standard browser controls (e.g., right-click > "Open in new tab")
- A mechanism exists to disable automatic context changes[1][4][9].
Implementation Guidelines
- Avoid
target="_blank"by default: Most links should open in the same tab[3][4][6]. Users can choose to open new tabs via browser controls[3].- Required warnings when using new tabs: If you must open a link in a new tab:
- Add visible icons (e.g., Font Awesome's external link icon)[1][6]
- Include screen-reader text like
<span class="screen-reader-text"> opens new window</span>[1][6]- Use ARIA labels for image links:
<a href="#" target="_blank" aria-label="Example (opens new tab)"> <img src="image.jpg" alt="Example"> </a> ```[7]Exceptions Allowing New Tabs
Scenario Example User progress loss risk Forms with external policy links[1][4] Required reference material Checklists/instructions needing parallel viewing[4] Best Practices
- Use descriptive link text avoiding "click here" or URLs[2][5][8]
- Maintain ≥3:1 contrast between link/non-link text[10]
- Implement hover/focus indicators for keyboard users[10]
- Automate warnings with plugins like Accessibility New Window Warnings[1]
Non-compliant implementations risk failing WCAG at AA/AAA levels and creating disorientation for screen reader users[1][3][4]. Government sites like accessibility.gov demonstrate compliant implementations with explicit warnings[9].
Citations:
- 1: https://theadminbar.com/accessibility-weekly/opening-links-in-new-tabs-or-not/
- 2: https://accessiblyapp.com/web-accessibility/links/
- 3: https://blog.pope.tech/2024/01/02/how-to-make-external-links-accessible/
- 4: https://websites.ucsc.edu/news/when-links-should-open-in-a-new-tab-or-window/
- 5: https://www.wcag.com/blog/writing-meaningful-link-text/
- 6: https://marcom.wwu.edu/accessibility/guide/links-should-open-same-tabwindow
- 7: https://www.discovertec.com/blog/ensure-links-explain-they-open-in-a-new-tab
- 8: https://www.w3.org/WAI/WCAG22/Understanding/link-purpose-in-context.html
- 9: https://equalizedigital.com/accessibility-checker/link-opens-new-window-or-tab/
- 10: https://webaim.org/techniques/hypertext/link_text
Accessibility Improvement Required: Enhance ARIA Label & Localization
Please update the Link component in src/components/Facility/FacilityMapLink.tsx (lines 17-44) to better notify users—especially screen reader users—when a link will open in a new tab/window. According to WCAG guidelines, links that open automatically in a new context should clearly indicate this behavior to prevent unexpected context changes.
Add an ARIA label:
Include an ARIA label that not only reinforces the localized link text but also provides additional context regarding the facility’s location. For example, consider updating your code as follows:<Link className="text-primary flex items-center gap-1 w-max" href={href} target={target} rel="noreferrer" + aria-label={`${t("show_on_maps")} for ${t("facility_location")}`} >Verify localization:
Ensure that the translation keys (t("show_on_maps")andt("facility_location")) remain clear and meaningful in all supported languages. Additionally, for users on non-Android devices (wheretarget="_blank"is used), verify that the implicit behavior of opening a new tab is either evident from the link text/icon or further clarified in the ARIA label.WCAG Consideration:
As the WCAG guidelines recommend warning users about links opening in new contexts, double-check if a visible indicator or additional screen-reader text (e.g., “opens in a new tab”) would further enhance usability.src/pages/Facility/components/FacilityCard.tsx (1)
40-45: Consider improving the coordinate type handling.The current implementation requires type conversion from number to string. Consider:
- Using string types in the facility model, or
- Moving the type conversion to the FacilityMapsLink component
- <FacilityMapsLink - latitude={facility.latitude.toString()} - longitude={facility.longitude.toString()} - /> + <FacilityMapsLink + latitude={facility.latitude} + longitude={facility.longitude} + />And update the FacilityMapsLink component to handle number types:
interface Props { latitude: string | number; longitude: string | number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
public/locale/en.json(1 hunks)src/Utils/utils.ts(1 hunks)src/components/Facility/FacilityHome.tsx(2 hunks)src/components/Facility/FacilityMapLink.tsx(1 hunks)src/pages/Facility/FacilityDetailsPage.tsx(2 hunks)src/pages/Facility/components/FacilityCard.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Facility/FacilityDetailsPage.tsx (1)
119-124: Apply consistent coordinate type handling.Similar to the FacilityCard component, consider improving the coordinate type handling here as well.
src/components/Facility/FacilityHome.tsx (1)
38-38: LGTM! Clean implementation of the map link feature.The changes properly integrate the FacilityMapsLink component with appropriate conditional rendering based on coordinate availability.
Also applies to: 318-325
public/locale/en.json (1)
2012-2012: LGTM! Proper localization key addition.The new localization entry follows naming conventions and is properly placed in alphabetical order.
rithviknishad
added
changes required
invalid
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care.config.ts (1)
58-59: Consider making the default map URL more configurable.While the implementation is correct, consider the following improvements:
- Move the default URL to a constant or configuration file to make it easier to update.
- Consider regional variations where Google Maps might not be the preferred or accessible mapping service.
Apply this diff to improve configurability:
+const DEFAULT_MAP_URL = "https://maps.google.com/?q="; + const careConfig = { // ... other config - mapUrl: env.REACT_MAP_URL || "https://maps.google.com/?q=", + mapUrl: env.REACT_MAP_URL || DEFAULT_MAP_URL, // ... other config } as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.example.env(1 hunks)care.config.ts(1 hunks)src/components/Facility/FacilityMapLink.tsx(1 hunks)src/vite-env.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .example.env
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/FacilityMapLink.tsx
🔇 Additional comments (1)
src/vite-env.d.ts (1)
36-37: LGTM!The new environment variable
REACT_MAP_URLfollows the established naming convention, has the correct type annotation, and is appropriately marked as optional.
|
|
||
| readonly REACT_MAP_URL?: string; |
There was a problem hiding this comment.
| readonly REACT_MAP_URL?: string; | |
| readonly REACT_MAPS_FALLBACK_URL_TEMPLATE?: string; |
| # Map URL | ||
| REACT_MAP_URL= |
There was a problem hiding this comment.
the purpose of example.env is to provide an example, where's the example value? 🤔
| @@ -55,6 +55,8 @@ const careConfig = { | |||
| defaultEncounterType: (env.REACT_DEFAULT_ENCOUNTER_TYPE || | |||
| "hh") as EncounterClass, | |||
|
|
|||
| mapUrl: env.REACT_MAP_URL || "https://maps.google.com/?q=", | |||
There was a problem hiding this comment.
let's keep the default in .env instead.
also let's use {lat}, {lng} in the env string which would be replaced by
| mapUrl: env.REACT_MAP_URL || "https://maps.google.com/?q=", | |
| mapUrl: (lat, lng) => env.REACT_MAP_URL.replace(...replace lat and lng with args...), |
There was a problem hiding this comment.
or better, just export the map url template in care config and have a common utility function to have this function, let's not keep functions in care config.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit