Add proceed anyway to small device message#2310
Conversation
WalkthroughThis pull request introduces a new static method Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JS
participant WP_Handler
participant Controller
User->>JS: Clicks "Proceed Anyway" button
JS->>WP_Handler: Sends AJAX POST request (frm_small_screen_proceed)
WP_Handler->>Controller: Invoke FrmAppController::small_screen_proceed
Controller->>Controller: Check permissions, verify nonce, update user option
Controller-->>WP_Handler: Return JSON success response
WP_Handler-->>JS: Transmit response
JS->>JS: Remove small-device message container from UI
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 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: 0
🧹 Nitpick comments (2)
css/frm_admin.css (1)
9305-9308: CSS Selector Enhancement for Small Device UIThe new selectors using the
:has()pseudo-class target both.toplevel_page_formidable:has(#frm_small_device_message_container) #posts-filterand.post-type-frm_display:has(#frm_small_device_message_container) #posts-filterto hide the posts filter element when the small device message container is present. This modification aligns well with the PR objective of providing a “Proceed Anyway” option by decluttering the interface on small devices.Recommendations:
- Browser Compatibility: Although the
:has()pseudo-class is very powerful and expressive, its support in older browsers can be limited. Please verify that the target audience’s browsers fully support this feature or consider adding fallbacks or documentation to address potential compatibility issues.- Documentation: It might be beneficial to include a brief comment in this CSS block to explain the intended behavior for future maintainers.
classes/views/shared/small-device-message.php (1)
20-20: Implementation of dual-button interface works well, with a few enhancement opportunities.The new "Proceed Anyway" button provides the functionality described in the PR objectives. The implementation correctly includes:
- A unique ID for JavaScript interaction
- Proper escaping for the "Go Back" URL
- Clear, translatable button text
Some suggestions for improvement:
- <div><a href="<?php echo esc_url( admin_url() ); ?>" class="frm-button-primary"><?php esc_html_e( 'Go Back', 'formidable' ); ?></a> <a id="frm_small_screen_proceed_button" href="#" class="frm-button-secondary"><?php esc_html_e( 'Proceed Anyway', 'formidable' ); ?></a></div> + <div class="frm-button-group"> + <a href="<?php echo esc_url( admin_url() ); ?>" class="frm-button-primary"><?php esc_html_e( 'Go Back', 'formidable' ); ?></a> + <a id="frm_small_screen_proceed_button" href="#" class="frm-button-secondary" aria-label="<?php esc_attr_e( 'Proceed to Formidable Forms despite small screen warning', 'formidable' ); ?>"><?php esc_html_e( 'Proceed Anyway', 'formidable' ); ?></a> + </div>This refactoring:
- Uses a container class for button spacing (rather than non-breaking spaces)
- Improves accessibility with an aria-label
- Makes the code more readable with better formatting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmAppController.php(1 hunks)classes/controllers/FrmHooksController.php(1 hunks)classes/views/shared/small-device-message.php(1 hunks)css/frm_admin.css(1 hunks)js/formidable_admin.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
classes/controllers/FrmAppController.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4474)permission_check(2063-2071)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (5)
classes/controllers/FrmAppController.php (1)
1511-1523: LGTM - New functionality to allow users to proceed on small screens.This new method handles the AJAX request when users click "Proceed Anyway" on small screens. The implementation is secure, checking permissions and verifying nonce before updating user preferences.
classes/controllers/FrmHooksController.php (1)
305-305: LGTM - Hook correctly registers the AJAX action.The hook appropriately connects the AJAX action to the newly added small screen proceed method. This follows the pattern used for other AJAX actions in this file.
js/formidable_admin.js (1)
10642-10649: Well-implemented "Proceed Anyway" button functionality.This implementation adds a click handler for the small screen proceed button that removes the warning message and sends an AJAX request to save the user's preference. The code follows good practices by checking if the element exists before attaching an event handler and using
onClickPreventDefaultto handle the click event properly.classes/views/shared/small-device-message.php (2)
13-14: User preference check looks good!The conditional check for whether a user has already chosen to ignore the small screen warning is properly implemented using WordPress's
get_user_option()function.
23-24: Proper PHP structureThe PHP closing tag and conditional block closure are correctly placed, ensuring the entire message is conditionally displayed based on user preference.
AbdiTolesa
left a comment
AbdiTolesa
left a comment
There was a problem hiding this comment.
Thanks @Crabcyborg! This looks good to me.
|
Thank you Abdi! |
2 participants
Related comment https://github.com/Strategy11/formidable-pro/issues/4076#issuecomment-2790657289
The issue - Some users actually use the builder on their phones. So far I've seen this come up in 2 tickets, both on the day after the release.
Updated design https://www.figma.com/design/DQbZMgeNmJWkxOWYfHjDZX/Formidable-System?node-id=316-1642
This update adds a "Proceed Anyway" button to the small device message, instead of blocking their access to the pages. I save this as a user pref so it isn't shown again.