Add required addons to application modal#2107
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@Crabcyborg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (5)
classes/models/FrmApplicationTemplate.php (1)
201-206: Consider adding type validation for used_addonsWhile the implementation follows the existing patterns and correctly handles the case conversion, consider adding type validation to ensure robustness:
$application['usedAddons'] = array(); if ( isset( $application['used_addons'] ) ) { + // Ensure used_addons is an array to prevent potential type errors + $used_addons = (array) $application['used_addons']; - $application['usedAddons'] = $application['used_addons']; + $application['usedAddons'] = $used_addons; unset( $application['used_addons'] ); }Also, consider adding a PHPDoc comment to document the purpose of the usedAddons property in the resulting JavaScript object.
js/admin/applications.js (4)
453-461: Use optional chaining for safer array access.The check for
data.usedAddons && data.usedAddons.lengthcould be simplified using optional chaining.-if ( data.usedAddons && data.usedAddons.length ) { +if ( data.usedAddons?.length ) {🧰 Tools
🪛 Biome
[error] 453-453: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
457-457: Inconsistent text domain usage.The text domain 'formidable-pro' is used here while 'formidable' is used elsewhere in the file. Consider maintaining consistency with the 'formidable' text domain.
-text: __( 'Required Add-ons', 'formidable-pro' ) +text: __( 'Required Add-ons', 'formidable' )
450-450: Add null check for description.Consider adding a null check for
data.descriptionto handle cases where the description might be undefined.-div( data.description ) +div( data.description || __( 'No description available', 'formidable' ) )
483-491: Enhance list utility function robustness.Consider improving the
getBasicListfunction with better error handling and flexibility.-function getBasicList( data ) { +function getBasicList( data, className = 'frm-application-item-list' ) { + if ( !Array.isArray(data) || !data.length ) { + return null; + } return tag( 'ul', { - className: 'frm-application-item-list', - children: data.map( text => tag( 'li', text ) ) + className, + children: data.map( text => tag( 'li', text?.toString() || '' ) ) } ); }This enhancement:
- Adds input validation
- Makes the class name configurable
- Handles potential non-string items
- Returns null for empty/invalid input
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
classes/models/FrmApplicationTemplate.php(2 hunks)js/admin/applications.js(2 hunks)
🧰 Additional context used
🪛 Biome
js/admin/applications.js
[error] 453-453: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
classes/models/FrmApplicationTemplate.php (1)
42-42: LGTM! Addition of 'used_addons' key
The new key follows the existing naming convention and integrates well with the filtering system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
css/admin/applications.css (2)
188-190: Consider alternatives to using !importantWhile setting a max-height constraint is reasonable for modal content, using
!importantshould be avoided as it:
- Makes styles harder to override when needed
- Could cause maintenance issues
- May impact accessibility for users with larger font sizes or zoom levels
Consider these alternatives:
-#frm_view_application_modal .postbox { - max-height: 800px !important; -} +/* Option 1: Use more specific selector */ +#frm_view_application_modal .postbox.frm-dialog-content { + max-height: 800px; +} + +/* Option 2: Use CSS custom property for flexibility */ +:root { + --frm-modal-max-height: 800px; +} +#frm_view_application_modal .postbox { + max-height: var(--frm-modal-max-height); +}
188-191: Consider restructuring modal stylesThe modal styles are scattered throughout the file. Consider:
- Grouping all modal-related styles together
- Using BEM or similar naming convention for better organization
- Enhancing responsive design for the modal
Example structure:
/* Modal Layout */ .frm-modal { } .frm-modal__header { } .frm-modal__content { } .frm-modal__footer { } /* Modal Variations */ .frm-modal--application { } /* Modal Responsive Design */ @media (max-width: 800px) { .frm-modal { } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
css/admin/applications.css(1 hunks)
🔇 Additional comments (1)
css/admin/applications.css (1)
188-190: Verify content overflow handling
With the addition of application descriptions and required add-ons, the 800px height limit might be too restrictive. Please ensure that:
- Long content remains accessible through proper scrolling
- The modal remains usable on mobile devices
- The content doesn't get cut off abruptly
Let's check if there are any overflow handling styles:
✅ Verification successful
Content overflow is properly handled
The modal already implements proper overflow handling through .frm_modal_content { overflow-y: auto; }. The 800px height limit on the postbox serves as a reasonable maximum container size, while the scrollable content area ensures all content remains accessible regardless of length.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for overflow-related styles for the modal
rg -l 'overflow|scroll' --type css
# Check for responsive design handling of the modal
rg -l '@media.*frm_view_application_modal' --type css
Length of output: 350
Script:
#!/bin/bash
# Check for overflow-related styles specifically for the view application modal
rg 'overflow.*frm_view_application_modal|frm_view_application_modal.*overflow' --type css
# Check for scroll-related styles specifically for the view application modal
rg 'scroll.*frm_view_application_modal|frm_view_application_modal.*scroll' --type css
# Check for any other styles related to the view application modal
rg '#frm_view_application_modal' -A 5 -B 5 --type css
Length of output: 2399
|
@srwells I have a small question for you, Do you think it's fine if we list required add-ons above the Forms, Views, Pages sections? Those additional sections are pulled in Pro only, by reading the data in the XML file. In Lite, we don't have access to the XML so we can't show those details. However, since "Required Add-ons" is included in our API data, we already have access to it on load for every template so I can show it immediately when you open the modal without the additional request for XML data that we do in Pro. As a result, this section is added first, and then those other sections get added later. If we want this section to be below the others, I'll need to update the code in Pro so it doesn't just append to the details - it would need to guarantee that everything is before the Required Add-ons section. Also, a second question - Do you think "Required Add-ons" is a good header? The design previously had "Dependencies" but I feel that sounds too technical and not descriptive enough. Thank you! 
|
|
Yes, I think it is fine for the required add-ons to be above the Forms/Views/Pages section. I actually prefer it. And I like "Required Add-ons" better than Dependancies, so we are good there. This will be a nice enhancement. |
|
Thanks Steve! I'll go ahead and merge this then. We'll likely revisit this and try to install the add-ons in the future, but I think just displaying them is a good start with minimal effort. 🚀 |
2 participants
No description provided.