Fix Captcha not rendering in bs modal #2055
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2055 +/- ##
=========================================
Coverage 27.32% 27.33%
- Complexity 8104 8110 +6
=========================================
Files 127 127
Lines 26918 26930 +12
=========================================
+ Hits 7356 7361 +5
- Misses 19562 19569 +7 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable.js (1)
1863-1872: LGTM! Captcha rendering logic improved for modal support.The changes address the issue of Captcha not rendering in Bootstrap modals. The new implementation correctly checks for forms within modals and visible forms, which should resolve the reported problem.
A minor suggestion for improved readability:
Consider extracting the visibility check into a separate function for better readability. For example:
- let shouldRenderCaptcha; - if ( closestForm.closest( '[id^=frm-modal-]' ) ) { // Form is in a modal. - shouldRenderCaptcha = true; - } else { - shouldRenderCaptcha = closestForm.offsetParent !== null; // Form is visible if offsetParent is not null. - } + const shouldRenderCaptcha = isFormVisibleOrInModal(closestForm); + function isFormVisibleOrInModal(form) { + return form.closest('[id^=frm-modal-]') || form.offsetParent !== null; + }This change would make the main function more concise and easier to understand at a glance.
|
@lauramekaj1 I see an error in the E2E test, I appreciate if you can look into it. Let me know should you have any question or help from me. |
|
@AbdiTolesa I think the failing e2e tests were because of some update I made the other day. I just merged the latest version of master into your branch. It should pass now. |
|
@AbdiTolesa @lauramekaj1 We went with another way to fix this issue and other similar issues #2058 I'm going to close this one. Don't worry about testing it Laura. The other fix is verified by Ahmed and myself. I think it's been tested enough. |
Fix https:/Strategy11/formidable-modal/issues/46
Relevant knowledge base
https://formidableforms.com/knowledgebase/bootstrap-modal/
Test Procedure
You can create reCaptcha keys here: https://www.google.com/recaptcha/
Formidable Modalblock, select the block and add aFormidable Formblock to the modal block. Then select the form you just created from theFormidable Formblock options on the side bar.