Make admin nav responsive #2241
Conversation
WalkthroughThe changes introduce new styles in the CSS file for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PHPStan (2.0.3)/bin/bash: line 1: /vendor/bin/phpstan: No such file or directory Tip 🌐 Web search-backed reviews and chat
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
🧹 Nitpick comments (2)
css/frm_admin.css (2)
8788-8791: Adjust publishing button alignmentThe margin-left: auto on #frm-publishing could be made more explicit with flexbox:
-#frm-publishing { - margin-left: auto; -} +#frm-publishing { + display: flex; + justify-content: flex-end; + flex: 1; +}This makes the alignment intention clearer and provides better browser compatibility.
8789-8791: Improve last child margin handlingThe margin-left: 0 on the last child could be handled more elegantly:
-#frm_top_bar > .frm-full-close:last-child { - margin-left: 0; -} +#frm_top_bar > :last-child { + margin-left: 0; +}This makes the rule more generic and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
css/frm_admin.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run ESLint
🔇 Additional comments (1)
css/frm_admin.css (1)
8774-8791: Consider accessibility implicationsThe fixed navigation at the bottom may cause accessibility issues:
- Verify that keyboard navigation works properly with the fixed positioning
- Ensure sufficient touch targets for mobile users
- Consider adding aria-labels for screen readers
- Test with different screen sizes and orientations
Run the following to check for potential accessibility issues:
| #frm_top_bar ul.frm_form_nav { | ||
| padding: 1em 0; | ||
| position: fixed; | ||
| bottom: 0; | ||
| width: 100%; | ||
| background: white; | ||
| z-index: 99; | ||
| transform: translate(-50%, 0); | ||
| left: 50%; | ||
| } | ||
|
|
||
| #frm-publishing { | ||
| margin-left: auto; | ||
| } | ||
|
|
||
| #frm_top_bar > .frm-full-close:last-child { | ||
| margin-left: 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add responsive navigation bar styles for mobile view
The changes add styles for a fixed navigation bar at the bottom of the screen on mobile devices. This improves mobile usability but there are a few considerations:
- The fixed positioning with
transform: translate(-50%, 0)andleft: 50%could be simplified. Consider using:
- transform: translate(-50%, 0);
- left: 50%;
+ left: 0;
+ right: 0;- The z-index of 99 may need to be higher to ensure the navigation stays above all content. Consider:
- z-index: 99;
+ z-index: 999;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #frm_top_bar ul.frm_form_nav { | |
| padding: 1em 0; | |
| position: fixed; | |
| bottom: 0; | |
| width: 100%; | |
| background: white; | |
| z-index: 99; | |
| transform: translate(-50%, 0); | |
| left: 50%; | |
| } | |
| #frm-publishing { | |
| margin-left: auto; | |
| } | |
| #frm_top_bar > .frm-full-close:last-child { | |
| margin-left: 0; | |
| } | |
| #frm_top_bar ul.frm_form_nav { | |
| padding: 1em 0; | |
| position: fixed; | |
| bottom: 0; | |
| width: 100%; | |
| background: white; | |
| z-index: 999; | |
| left: 0; | |
| right: 0; | |
| } | |
| #frm-publishing { | |
| margin-left: auto; | |
| } | |
| #frm_top_bar > .frm-full-close:last-child { | |
| margin-left: 0; | |
| } |
| #frm-publishing { | ||
| margin-left: auto; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify navigation bar background contrast
The white background may not provide enough contrast with the content below it. Consider adding a subtle box shadow to visually separate it:
background: white;
+ box-shadow: 0 -2px 4px rgba(0,0,0,0.1);Error: No shell scripts or web queries found
🏁 Script executed:
#!/bin/bash
# Retrieve context around the #frm-publishing rule in css/frm_admin.css to check for background and box-shadow properties
rg -n -C 5 "#frm-publishing" css/frm_admin.cssLength of output: 1861
🏁 Script executed:
#!/bin/bash
# Searching for 'background: white' in the repository to determine if it's used in the navigation bar styles.
rg -n "background:\s*white" .Length of output: 509
Navigation Bar Contrast Improvement: Add a Subtle Box-Shadow
- The CSS in several files (e.g. in
css/frm_admin.cssat line 8779) applies a white background to elements in the navigation area. - The rule for
#frm-publishing(lines 8785–8787) does not include any box-shadow property, which could help visually separate the navigation bar from underlying content. - Adding a subtle box-shadow such as
box-shadow: 0 -2px 4px rgba(0,0,0,0.1);
would enhance the contrast and improve visual separation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
classes/views/frm-forms/edit.php (1)
8-21: LGTM! Good implementation with some suggested improvements.The small device message container is well-structured and properly internationalized. Here are some suggested improvements:
- The "Go back" button should have a proper destination or use JavaScript event handler instead of "#"
- Add ARIA attributes to make the SVG icon accessible
- Consider including the minimum recommended screen size in the message
Apply these improvements:
-<svg width="32" height="49" viewBox="0 0 32 49" fill="none" xmlns="http://www.w3.org/2000/svg"> +<svg width="32" height="49" viewBox="0 0 32 49" fill="none" xmlns="http://www.w3.org/2000/svg" role="img" aria-label="Mobile device icon">-<a href="#" class="frm-button-primary"><?php esc_html_e( 'Go back', 'formidable' ); ?></a> +<button type="button" onclick="window.history.back()" class="frm-button-primary"><?php esc_html_e( 'Go back', 'formidable' ); ?></button>-<p><?php esc_html_e( 'For the best experience, we recommend using Formidable Forms on larger devices such as a desktop or tablet.', 'formidable' ); ?></p> +<p><?php esc_html_e( 'For the best experience, we recommend using Formidable Forms on devices with screens larger than 475px, such as a desktop or tablet.', 'formidable' ); ?></p>css/frm_admin.css (1)
8796-8802: Inconsistent margin handling in responsive layoutThe margin reset for the last child element is inconsistent with the rest of the responsive styles.
Consider standardizing margin handling:
#frm_top_bar > .frm-full-close:last-child { - margin-left: 0; + margin: 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/views/frm-forms/edit.php(1 hunks)css/frm_admin.css(4 hunks)
🔇 Additional comments (3)
css/frm_admin.css (3)
8623-8625: LGTM: Base class for small device message containerThe base class
.frm_small_device_message_containeris properly defined withdisplay: noneas the default state.
8653-8661: Verify Bootstrap dropdown visibilityThe code hides the Bootstrap dropdown and its tooltip on smaller screens. We should verify this doesn't break any critical functionality.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any references to #frm_bs_dropdown to ensure hiding it won't break functionality rg -l "frm_bs_dropdown" --type html --type php --type jsLength of output: 152
Bootstrap Dropdown Visibility Verified
The
#frm_bs_dropdownelement is referenced in a few places (in a PHP helper, a header file, and the admin JavaScript), and its usage is consistent with responsive design adjustments. Hiding it on smaller screens does not appear to break any critical functionality.
8778-8795: 🛠️ Refactor suggestionResponsive navigation bar styles need improvement
The navigation bar styles for mobile view have a few issues:
- The fixed positioning with
transform: translate(-50%, 0)andleft: 50%is unnecessarily complex.- The z-index of 99 may be too low to ensure the navigation stays above all content.
Apply this diff to improve the mobile navigation styles:
#frm_top_bar ul.frm_form_nav { padding: 1em 0; position: fixed; bottom: 0; width: 100%; background: white; z-index: 999; - transform: translate(-50%, 0); - left: 50%; + left: 0; + box-shadow: 0 -2px 4px rgba(0,0,0,0.1); }Likely invalid or redundant comment.
| .frm_small_device_message_container { | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| justify-content: center; | ||
| position: fixed; | ||
| top: 0; | ||
| width: 100%; | ||
| height: 100%; | ||
| z-index: 100; | ||
| background: var(--primary-500); | ||
| } | ||
|
|
||
| .frm_small_device_message { | ||
| text-align: center; | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| width: 80%; | ||
| padding: 1rem; | ||
| background: #fff; | ||
| border: solid 1px #fff; | ||
| border-radius: var(--medium-radius); | ||
| } | ||
|
|
||
| .frm_small_device_message::before { | ||
| content: ''; | ||
| position: absolute; | ||
| top: 50%; | ||
| left: 50%; | ||
| transform: translate(-78%, -65%); | ||
| width: 900px; | ||
| height: 900px; | ||
| background-color: #0D7DFC; | ||
| border-radius: 50%; | ||
| z-index: -1; | ||
| opacity: 0.3; | ||
| } | ||
|
|
||
| .frm_small_device_message > svg { | ||
| padding-bottom: 1rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Small device message container needs accessibility improvements
The small device message container has some accessibility concerns:
- The fixed positioning may trap keyboard focus
- Missing ARIA attributes for screen readers
- The background overlay lacks proper contrast ratio
Apply these improvements:
.frm_small_device_message_container {
display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
position: fixed;
top: 0;
width: 100%;
height: 100%;
z-index: 100;
background: var(--primary-500);
+ role="dialog"
+ aria-modal="true"
+ aria-label="Device size warning"
}
.frm_small_device_message {
text-align: center;
display: flex;
flex-direction: column;
align-items: center;
width: 80%;
padding: 1rem;
background: #fff;
border: solid 1px #fff;
border-radius: var(--medium-radius);
+ color: var(--grey-900);
+ box-shadow: 0 2px 4px rgba(0,0,0,0.2);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/controllers/FrmFormsController.php (1)
1898-1898: Consider conditional server-side rendering of the device message.While the current implementation works, consider conditionally rendering the message based on device detection to avoid unnecessary DOM nodes on desktop devices. This would improve performance and reduce page weight.
- self::include_device_too_small_message(); + if ( wp_is_mobile() ) { + self::include_device_too_small_message(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmFormsController.php(1 hunks)classes/views/frm-forms/edit.php(0 hunks)css/frm_admin.css(6 hunks)
💤 Files with no reviewable changes (1)
- classes/views/frm-forms/edit.php
🚧 Files skipped from review as they are similar to previous changes (1)
- css/frm_admin.css
🔇 Additional comments (1)
classes/controllers/FrmFormsController.php (1)
1869-1892: Well-structured implementation of the device message component!The implementation follows best practices with:
- Proper escaping of translations
- Semantic HTML structure
- Accessibility considerations with role="status"
- Clear visual hierarchy in the UI components
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
css/frm_admin.css (1)
8787-8789: Improve publishing button positioning-#frm-publishing { - margin-left: auto; -} +#frm-publishing { + margin-left: auto; + margin-right: var(--gap-sm); +}Add right margin for better spacing and alignment with other elements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmEntriesController.php(1 hunks)classes/controllers/FrmStylesController.php(1 hunks)css/frm_admin.css(6 hunks)
🔇 Additional comments (7)
classes/controllers/FrmStylesController.php (1)
863-863: LGTM! Verify message visibility logic.The addition of the device message before action routing is well-placed. The message will be consistently available across all routes, with visibility likely controlled via CSS based on device size.
Let's verify the CSS breakpoints that control the message visibility:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for CSS rules that control the visibility of the device message container rg -A 5 "frm_small_device_message_container"Length of output: 1692
Based on the output from the CSS search, the message container (
frm_small_device_message_container) is styled in two different blocks. One rule hides the container (usingdisplay: none;), and another sets it to a flex container. This indicates that the visibility is controlled by CSS (likely via media queries or other context-specific rules) so that the message only appears on small devices as intended.LGTM! The change in
FrmStylesController.phpcorrectly calls the method to include the device message, and CSS rules confirm that its visibility is appropriately managed. Please verify that the media query conditions correctly trigger the “display: flex” rule on small devices.classes/controllers/FrmEntriesController.php (1)
113-113: LGTM! Verify the implementation of the device message.The addition of the device size message aligns with the PR objectives for responsive styles.
Let's verify the implementation of the called method:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of include_device_too_small_message() # Search for the method definition ast-grep --pattern 'public static function include_device_too_small_message() { $$$ }' # Search for other usages of this method rg -A 5 'include_device_too_small_message'Length of output: 2232
LGTM! The device message implementation is correctly integrated.
- The method
include_device_too_small_message()is implemented inclasses/controllers/FrmFormsController.phpand outputs the expected HTML (including the SVG icon) for a small device message.- The new call in
FrmEntriesController.php(and similar usage inFrmStylesController.php) confirms consistent usage across the codebase.css/frm_admin.css (5)
8623-8626: Add small device message container stylesThe styles define a container for displaying messages on small devices.
The implementation follows best practices by:
- Using display: none by default
- Using semantic class names
- Keeping styles modular and reusable
8656-8659: Hide tooltip on smaller screensGood practice to hide the tooltip when there isn't enough screen space to display it properly. The
!importantflag is justified here to ensure the tooltip stays hidden.
9060-9102: 🛠️ Refactor suggestionImprove small device message container layout and accessibility
.frm_small_device_message_container { display: flex; flex-direction: column; align-items: center; justify-content: center; position: fixed; top: 0; width: 100%; height: 100%; z-index: 100; background: var(--primary-500); + role: "dialog"; + aria-modal: "true"; + aria-label: "Device size warning"; }Add ARIA attributes to improve accessibility for screen readers.
Likely invalid or redundant comment.
8776-8793: 🛠️ Refactor suggestionRefactor navigation bar styles for mobile view
The changes add styles for a fixed navigation bar at the bottom of the screen on mobile devices. The implementation can be improved for better maintainability and performance.
-#frm_top_bar ul.frm_form_nav { - padding: 1em 0; - position: fixed; - bottom: 0; - width: 100%; - background: white; - z-index: 99; - transform: translate(-50%, 0); - left: 50%; -} +#frm_top_bar ul.frm_form_nav { + padding: var(--gap-sm) 0; + position: fixed; + bottom: 0; + left: 0; + right: 0; + width: 100%; + background: #fff; + z-index: 99; +}The changes:
- Use CSS variables for consistent padding
- Remove transform/left positioning in favor of left/right: 0
- Use #fff instead of white for consistency
Likely invalid or redundant comment.
8794-8800: Verify dropdown button display propertiesThe dropdown button display property is being modified. Need to verify this won't affect other uses of the dropdown button in the codebase.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the dropdown button is used elsewhere in the codebase rg -l "frm_bs_dropdown" .Length of output: 176
We’ve confirmed that the
#frm_bs_dropdownidentifier is referenced in several files:
./classes/helpers/FrmFormsHelper.php./classes/views/applications/header.php./css/admin/applications.css./css/frm_admin.css./js/formidable_admin.jsBefore finalizing the verification, we need to review the context in which
#frm_bs_dropdownis used in these files to ensure that forcing adisplay: block(and similarly for.frm_bstooltip) infrm_admin.cssdoesn’t unintentionally override styles needed elsewhere (for example, in the admin area versus public application views).Let's inspect the surrounding context for each occurrence. Please run the following shell script to show several lines around each match:
Once we review the context from these files, we can determine whether the forced
display: blockincss/frm_admin.cssis appropriate and isolated (for example, if it only applies to the admin interface) or if it might conflict with other areas of the codebase that rely on different behavior for the dropdown.
🏁 Script executed:
#!/bin/bash # Display occurrences with context for "#frm_bs_dropdown" in all related files FILES=( "css/admin/applications.css" "css/frm_admin.css" "classes/helpers/FrmFormsHelper.php" "classes/views/applications/header.php" "js/formidable_admin.js" ) for file in "${FILES[@]}"; do echo "=== $file ===" rg -C 3 "#frm_bs_dropdown" "$file" || echo "No matches found" echo "" doneLength of output: 2618
Review Confirmed: Dropdown Button Display Styles Are Intentional
After inspecting all occurrences:
- In
css/frm_admin.css, the dropdown button is initially set with conflicting tooltip visibility (first hidden then forced to display). The final state is ensured by the laterdisplay: block !importantrules.- In the admin-specific files (
css/frm_admin.css,js/formidable_admin.js, etc.), the styles are intended for the admin interface.- Other files (like
css/admin/applications.css) reference the element in contexts that do not conflict with this behavior.The forced
display: blockincss/frm_admin.cssappears to be a deliberate override for the admin interface and does not adversely affect other areas.
AbdiTolesa
changed the title
| */ | ||
| public static function include_device_too_small_message() { | ||
| ?> | ||
| <div class="frm_small_device_message_container"> |
There was a problem hiding this comment.
@AbdiTolesa Could we use an id instead? We only expect to have this element on a page once, right? This should make it easier to style as well. It would be nice to avoid stuff like :not(.frm_small_device_message_container) when possible. It makes maintenance difficult, and looks bad.
Fix https://github.com/Strategy11/formidable-pro/issues/4076
Design: https://www.figma.com/design/DQbZMgeNmJWkxOWYfHjDZX/Formidable-System?node-id=316-1642&t=w5JzVxzSrEUHXsVY-0