Collect feedback deactivation#1919
Conversation
WalkthroughThe recent updates introduce a new controller, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
participant Modal
User->>UI: Clicks on Deactivate
UI->>Modal: Open Deactivation Modal
Modal->>API: Fetch Deactivation Feedback HTML
API-->>Modal: Return Feedback HTML
Modal->>UI: Display Feedback Form
User->>Modal: Submit Feedback
Modal->>API: Send Feedback Submission
API-->>Modal: Confirmation Response
Modal->>UI: Close Modal
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
classes/controllers/FrmFormsController.php (1)
1194-1194: Consider removing or documenting commented-out code.The commented-out
error_logstatement is intended for debugging. If it's no longer needed, consider removing it to keep the code clean. Otherwise, add a comment explaining its purpose.
truongwp
requested review from
garretlaxton
and removed request for
garretlaxton
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
js/admin/deactivation-feedback.js (1)
21-48: Consider externalizing dialog configuration.To enhance maintainability, consider externalizing the dialog configuration options into a separate configuration object or file. This approach can simplify updates and provide a centralized location for UI settings.
// Example configuration object const dialogConfig = { dialogClass: 'frm-dialog', modal: true, autoOpen: false, closeOnEscape: true, width: '550px', resizable: false, draggable: false, open: function() { // existing open logic }, close: function() { // existing close logic } }; // Usage $info.dialog(dialogConfig);
|
I feel like there should be a The success message overlaps the Formidable icon. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
js/admin/deactivation-feedback.js (1)
92-105: Improve error handling for fetch response.Consider providing user feedback in the UI when an error occurs during the fetch request to enhance user experience.
.catch( error => { console.error( error ); + alert('An error occurred while fetching the feedback form. Please try again later.'); });
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
js/admin/deactivation-feedback.js (1)
2-2: Remove redundant 'use strict' directive.The 'use strict' directive is redundant in ES6 modules as they are in strict mode by default.
Apply this diff to remove the directive:
- 'use strict';Tools
Biome
[error] 2-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
|
Thanks @garretlaxton. I will fix the icon error. |
|
@garretlaxton I fixed the icon issue. Now the form will show once in 1 day. Please test it again. |
There was a problem hiding this comment.
This all looks great!
I haven't tested this yet but I will soon and merge this for next release. I plan to wait a day or two in case something gets reported in version 6.14 that needs a patch.
I left one comment about moving the CSS into a file. That's optional. I think we're fine merging this as-is.
🚀
|
@Crabcyborg Do not merge this before we update the expired time of the popup. Laura is also testing this. |
|
Hi @truongwp, I have done some testing on the deactivation feedback popup and here are some findings if you can review them!
|
|
Thanks @lauramekaj1. I fixed those problems in the feedback form. You won't need to update the code to test again. For the 1st: |
There was a problem hiding this comment.
I'll test this before I release it tomorrow.
But I'll go ahead and merge this now. It all looks good to me.
Thanks @truongwp, @garretlaxton, and @lauramekaj1 for working on this!
🙌
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
classes/controllers/FrmHooksController.php (1)
202-205: LGTM! Consider grouping related hooks for better organization.The addition of deactivation feedback hooks is well-implemented and follows the existing coding conventions. The use of standard WordPress hooks ensures compatibility.
For improved code organization, consider grouping these related hooks together at the beginning of the
load_admin_hooksmethod, just after theadd_action( 'admin_menu', 'FrmAppController::menu', 1 );line. This would make it easier to locate and manage all deactivation-related functionality in one place.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
classes/controllers/FrmDeactivationFeedbackController.php (1)
60-60: Consider storing expiration date with time to avoid unintended early expirationCurrently, the expiration date is stored using only the date in 'Y-m-d' format, which defaults the time to midnight when parsed with
strtotime(). This may result in the feedback being considered expired at the very start of the expiration day. If the intended behavior is to expire after exactly 24 hours, consider storing the full datetime to capture the precise expiration moment.Apply this diff to store the full datetime:
- update_option( 'frm_feedback_expired', gmdate( 'Y-m-d', strtotime( '+ 1 day' ) ) ); + update_option( 'frm_feedback_expired', gmdate( 'Y-m-d H:i:s', strtotime( '+1 day' ) ) );Then, adjust the
feedback_is_expired()method to handle the datetime format appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- classes/controllers/FrmDeactivationFeedbackController.php (1 hunks)
🔇 Additional comments (3)
classes/controllers/FrmDeactivationFeedbackController.php (3)
110-129: HTML output is properly escaped and structuredThe HTML output in the
footer_html()method is well-structured, and appropriate escaping functions likeesc_attr_e()andesc_url()are used to ensure security.
1-131: Overall code implementation looks solidThe
FrmDeactivationFeedbackControllerclass is well-designed, with clear method definitions handling the plugin deactivation feedback functionality effectively.
90-90: Verify script handle inlocalize_scriptmethodIn the
enqueue_assets()method, you are callingFrmAppHelper::localize_script( 'front' );. Please confirm that 'front' is the correct script handle for localization in this context. If you intend to localize the'frm-deactivation-feedback'script, you may need to adjust the handle accordingly.You can run the following script to check the script handles used in
localize_scriptcalls:
4 participants
Handoff: https://app.clickup.com/9006050445/v/dc/8ccuv4d-30651
Feedback form: https://feedback.strategy11.com/wp-admin/admin.php?page=formidable&frm_action=edit&id=20