-
-
Notifications
You must be signed in to change notification settings - Fork 329
feat: update map overlay to 'Unlock map' with unlock icon #2844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Changed overlay text from 'Click to interact with map' to 'Unlock map' - Added unlock icon (faLockOpen) from FontAwesome before text - Updated aria-label for better accessibility - Updated all related unit tests All tests passing (30/30)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe pull request updates the ChapterMap component's interactive overlay prompt text from "Click to interact with map" to "Unlock map" and adds a lock icon (faLockOpen) alongside it. Both the component and its unit tests are updated to reflect this change. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
268-316: Overlay interaction tests correctly updated to the new “Unlock map” labelThe Interactive Overlay tests now assert against “Unlock map” while still verifying all key behaviors (initial visibility, removal on click, scrollWheelZoom enabling, Enter/Space keyboard handling, and aria-label). This keeps the suite tightly aligned with the UI change without losing coverage. As an optional enhancement, you could add an assertion that the overlay still renders the icon if you want explicit regression coverage for the visual cue, but it’s not required for this change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx(4 hunks)frontend/src/components/ChapterMap.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (2)
frontend/src/components/ChapterMap.tsx (2)
5-6: FontAwesome icon imports align with intended usageThe
faLockOpenandFontAwesomeIconimports match how you’re using the unlock icon in the overlay and are consistent with standard@fortawesome/react-fontawesomepatterns; no issues from this change.If FontAwesome wasn’t already in use elsewhere, please double-check that the corresponding packages/versions are present in
package.jsonand installed so this doesn’t introduce a runtime import error.
149-154: Overlay label + icon change correctly implements “Unlock map” behaviorThe updated overlay keeps the existing interaction model (click + keyboard) while:
- Setting
aria-label="Unlock map"on the button, and- Rendering a flex row with
FontAwesomeIcon(faLockOpen)followed by the “Unlock map” text.This cleanly satisfies the PR/issue requirements without altering map behavior; a11y remains intact and arguably clearer.
kasya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swankystark Hi! Please run make check-test locally and address found issues ✔️
| <p className="flex items-center gap-2 rounded-md bg-white/90 px-5 py-3 text-sm font-medium text-gray-700 shadow-lg dark:bg-gray-700 dark:text-white"> | ||
| <FontAwesomeIcon icon={faLockOpen} className="h-4 w-4" /> | ||
| Unlock map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swankystark Could you also add some hover effect to this button?



Proposed change
Resolves #2839
Checklist