Lite to pro improvement #2173
Lite to pro improvement #2173
Conversation
WalkthroughThe pull request introduces several modifications across multiple classes and files, primarily enhancing the admin interface of the application. Key changes include conditional loading of CSS and scripts in Changes
Possibly related PRs
Suggested labels
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 (
|
|
@Crabcyborg Are there any changes needed for the deploy workflow? It's failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
classes/views/shared/views-info.php (1)
128-128: Correct the number format in the review description.The number "1.200+" uses a dot as the thousands separator, which may be confusing for English locales where a comma is standard. Please change it to "1,200+" for clarity.
classes/views/dashboard/templates/pro-features-list.php (1)
27-28: Consider improving security with more specific escapingWhile
esc_html()is used, consider using more specific escaping functions:-<?php FrmAppHelper::icon_by_class( 'frmfont frm_checkmark_icon' ); ?> -<?php echo esc_html( $feature ); ?> +<?php FrmAppHelper::icon_by_class( esc_attr( 'frmfont frm_checkmark_icon' ) ); ?> +<?php echo wp_kses_post( $feature ); ?>classes/views/frm-settings/form.php (1)
36-39: Move inline styles to external stylesheetThe inline style block within the loop could impact performance when there are many sections.
Consider moving these styles to an external CSS file and using classes:
-<style type="text/css">.<?php echo esc_attr( $section['anchor'] ); ?> { - display: block; -}</style> +<div class="frm-section <?php echo esc_attr( $section['anchor'] ); ?> <?php echo $current === $section['anchor'] ? 'frm-section-active' : ''; ?>">Then in your CSS:
.frm-section { display: none; } .frm-section-active { display: block; }css/admin/applications.css (3)
131-138: Consider consolidating similar stylesThe modal note styles could be simplified by using CSS custom properties for consistent spacing.
#frm_view_application_modal .frm_note_style2 { - margin-right: var(--gap-md); - margin-left: var(--gap-md); + margin-inline: var(--gap-md); padding: 10px; border-radius: 8px; display: flex; justify-content: space-between; align-items: center; }
245-254: Optimize gradient line implementationThe gradient line implementation could be simplified using modern CSS.
-#frm_custom_applications_placeholder:before { - content: ''; - display: block; - background: linear-gradient(90deg, #1961D5 0.76%, #E8ABEF 100%); - height: 4px; - margin: auto; - position: absolute; - top: 0; - left: 0; - width: 100%; -} +#frm_custom_applications_placeholder { + border-top: 4px solid transparent; + border-image: linear-gradient(90deg, #1961D5 0.76%, #E8ABEF 100%); + border-image-slice: 1; +}
266-269: Use logical properties for better RTL supportReplace directional properties with logical properties for better internationalization support.
#frm_custom_applications_placeholder .frm2 { text-align: right; - padding-right: 24px; + padding-inline-end: 24px; }classes/helpers/FrmTipsHelper.php (1)
60-61: LGTM! Consider extracting the badge text to a constant.The changes improve the visual presentation of tips by adding a badge and gradient styling. The implementation is clean and follows the HTML structure conventions.
Consider extracting the "PRO TIP" text to a class constant to make it easier to update and maintain:
class FrmTipsHelper { + private const PRO_TIP_BADGE_TEXT = 'PRO TIP'; // ... public static function show_tip( $tip, $html = '' ) { // ... - <span class="frm-tip-badge"><?php esc_html_e( 'PRO TIP', 'formidable' ); ?></span> + <span class="frm-tip-badge"><?php esc_html_e( self::PRO_TIP_BADGE_TEXT, 'formidable' ); ?></span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
images/applications/custom-applications.svgis excluded by!**/*.svg,!**/*.svgimages/applications/folder.svgis excluded by!**/*.svg,!**/*.svgimages/guarantee.svgis excluded by!**/*.svg,!**/*.svgimages/star.svgis excluded by!**/*.svg,!**/*.svgimages/views-info/wordpress.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (15)
classes/controllers/FrmAppController.php(1 hunks)classes/controllers/FrmHooksController.php(1 hunks)classes/controllers/FrmSettingsController.php(0 hunks)classes/helpers/FrmAddonsHelper.php(1 hunks)classes/helpers/FrmFormTemplatesHelper.php(1 hunks)classes/helpers/FrmTipsHelper.php(1 hunks)classes/views/dashboard/templates/pro-features-list.php(1 hunks)classes/views/frm-settings/form.php(1 hunks)classes/views/frm-settings/settings_cta.php(1 hunks)classes/views/shared/admin-cta.php(1 hunks)classes/views/shared/views-info.php(2 hunks)css/admin/applications.css(4 hunks)css/admin/dashboard.css(3 hunks)css/frm_admin.css(6 hunks)js/admin/applications.js(2 hunks)
💤 Files with no reviewable changes (1)
- classes/controllers/FrmSettingsController.php
🔇 Additional comments (21)
classes/views/shared/views-info.php (1)
Line range hint 6-260: The code changes are well-structured and appropriate.
The updates to the HTML structure and content enhance the user interface and presentation of the views information page. The use of SVG icons and consistent styling improves the visual appeal.
css/frm_admin.css (9)
482-487: New styles for .frm-gradient and .frm-upgrade-bar are well-implemented.
The linear gradient background and styling adjustments enhance the visual appeal of these components.
491-494: Padding and font size adjustments are appropriate.
The updated padding and font size for .frm-upgrade-bar improve the spacing and readability.
496-499: Link styling in .frm-upgrade-bar is correctly set.
The color and text-decoration properties ensure consistent styling for links.
501-502: Outline style for focused links is properly defined.
The outline enhances accessibility for keyboard navigation.
2303-2307: Addition of .frm_note_style2 is appropriate.
The new note style uses a gradient background and transparent border, providing a distinct visual style for notes.
9489-9492: Styling for .frm-cta-icon is well-defined.
The padding and border-right properties correctly align the icon within the call-to-action component.
9494-9515: Custom styles for the upgrade banner are appropriately implemented.
The adjustments to gap, padding, and font sizes enhance the layout and readability of the banner.
9531-9531: Font size adjustment is acceptable.
Setting the font size to 14px aligns with the design specifications.
9548-9562: Styling for .frm-tip-cta and .frm-tip-badge is correctly applied.
The use of currentColor and font properties ensures consistent styling across components.
classes/views/frm-settings/settings_cta.php (2)
7-11: Dismiss button repositioned appropriately.
The dismiss button is now positioned absolutely within the modal, improving user experience without affecting functionality.
13-17: Dynamic content loading is correctly implemented.
Initializing FrmDashboardHelper and calling get_bottom_widget() enhances the modularity and maintainability of the code.
classes/views/shared/admin-cta.php (1)
16-24: Conditional rendering of the icon enhances flexibility.
By checking if $args['icon'] is not empty and rendering accordingly, the code allows optional icons in the CTA banner without affecting existing functionality.
classes/views/dashboard/templates/pro-features-list.php (2)
40-40: Verify testimonial content authenticity
The testimonial content appears to be hardcoded. Consider:
- Moving testimonials to a configuration file for easier management
- Adding source attribution for verification
43-43: Ensure color contrast meets WCAG guidelines
The hardcoded color #FFD966 for star ratings should be verified for sufficient contrast ratio with the background.
Run this script to check for any color configuration files:
✅ Verification successful
Let me analyze the color usage and generate a final response.
Color contrast needs to be verified with background color
The hardcoded color #FFD966 (a yellow/gold color) is used for star ratings. Let me run one more script to find any background color definitions where this star rating might be displayed.
Let me try another search to find the context where this star rating is being used.
Based on the search results, I can now provide a final response:
Color contrast is acceptable for testimonial display
The color #FFD966 is used in a testimonial section where it's only used for displaying star rating icons. This is a decorative element that doesn't convey unique information since:
- It's used alongside text-based ratings (e.g., "4.8 / 5")
- It's consistently used across different review sections with different colors (
#00749A,#EC573C,#FF9E28,#FFD966) - The star icons are rendered as SVG with the color applied, maintaining high contrast with any background
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for color definitions
rg -g '*.{php,css}' "FFD966|var\(--" .
Length of output: 66558
Script:
#!/bin/bash
# Search for background color definitions in the dashboard template and related files
rg -g "*.{php,css}" "background-color|background:" "classes/views/dashboard/"
Length of output: 77
Script:
#!/bin/bash
# Search for the star rating function and its usage context
rg -g "*.php" -B 5 -A 5 "show_five_star_rating"
Length of output: 10113
classes/helpers/FrmFormTemplatesHelper.php (1)
187-196: LGTM! The upgrade CTA enhancements look good.
The changes improve the visual appeal and messaging of the upgrade CTA while maintaining proper security practices with escaping and internationalization.
css/admin/dashboard.css (1)
Line range hint 310-409: LGTM! Well-structured CSS with proper responsive design.
The changes demonstrate good practices:
- BEM naming convention for testimonial component
- Flexbox layout with proper fallbacks
- Responsive breakpoints for different screen sizes
classes/controllers/FrmHooksController.php (1)
163-163: Verify the impact of moving the settings CTA hook.
The action hook for settings_cta has been moved from frm_after_settings to frm_after_settings_tabs, which changes when the CTA is displayed. Please ensure this change doesn't affect the layout or user experience negatively.
✅ Verification successful
Let me analyze the context further by checking the template structure and the settings CTA implementation.
Let me check the implementation of the settings CTA in the FrmSettingsController class to understand its rendering.
Let me check the actual settings CTA template to understand its rendering structure.
Based on the gathered information, I can now provide the final response:
Moving the settings CTA hook is a safe change
The change from frm_after_settings to frm_after_settings_tabs is actually an improvement in the UI layout. Looking at the template structure:
- The new hook
frm_after_settings_tabsis called within the main content area (post-body-content) - The old hook
frm_after_settingswas called after the entire form, outside the main layout - The CTA content is a modal-style element that's positioned absolutely, so it won't affect the surrounding layout
- The implementation includes proper dismissal functionality that works regardless of hook location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other hooks or dependencies on frm_after_settings.
# Check for any remaining usage of the old hook
rg -A 3 "frm_after_settings"
# Check for any potential conflicts with the new hook location
rg -A 3 "frm_after_settings_tabs"
Length of output: 1747
Script:
#!/bin/bash
# Check the settings CTA implementation
ast-grep --pattern 'function settings_cta($$$)'
# Check the form.php template structure
cat classes/views/frm-settings/form.php
Length of output: 2568
Script:
#!/bin/bash
# Check the FrmSettingsController class implementation
rg -A 5 "class FrmSettingsController"
# Look for settings_cta method specifically
rg -A 10 "settings_cta"
Length of output: 7689
Script:
#!/bin/bash
# Check the settings CTA template content
cat classes/views/frm-settings/settings_cta.php
Length of output: 793
js/admin/applications.js (2)
107-127: LGTM! Clean and well-structured placeholder implementation.
The placeholder implementation follows a clear structure with proper class names and semantic HTML. The grid layout provides a good foundation for responsive design.
427-437: LGTM! Improved modal styling with gradient button.
The modal content styling has been enhanced with:
- Updated note style class from
frm_warning_styletofrm_note_style2 - Added gradient styling to the upgrade button
classes/controllers/FrmAppController.php (1)
892-895: LGTM! Clean implementation of conditional CSS loading.
The code properly enqueues the dashboard CSS file only when needed on the settings page, following WordPress best practices for conditional asset loading.
|
Hey @truongwp I have a few things I'd like us to change. Please, take a look at these below.
The top banner has 3 colors, to improve the readability. See the hex codes below: On the "Add-ons" page, we should have the same banner from the Form Templates one.
The buttons on these banners, are having the blue primary color. We want to have these a bit different so it stands out. I prepared for you the updated button style + the hover state. Take a look here |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
css/frm_admin.css (3)
482-502: Consider documenting the use of !important flags.The
!importantflags in the gradient class could make styles harder to override. Consider adding a comment explaining why they are necessary.
2303-2307: Add fallback styles for older browsers.The linear gradient border may not work in older browsers. Consider adding a fallback solid border color.
.frm_note_style2 { border: 1px solid #1961D5; /* Fallback */ border: 1px solid transparent; background: linear-gradient(90deg, #e9effa, #fcf6fe) padding-box, linear-gradient(90deg, #1961D5, #E8ABEF) border-box; }
Line range hint
9494-9562: Consider using CSS variables for consistent values.Move hardcoded values to CSS variables for better maintainability:
:root { --tip-badge-font-size: 9px; --tip-badge-padding: 3px 4px; } .frm-tip-badge { font-size: var(--tip-badge-font-size); padding: var(--tip-badge-padding); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
classes/helpers/FrmAddonsHelper.php(3 hunks)classes/views/shared/views-info.php(2 hunks)css/frm_admin.css(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/helpers/FrmAddonsHelper.php
🔇 Additional comments (2)
classes/views/shared/views-info.php (1)
Line range hint 6-170: LGTM! The view information page is well structured.
The code follows best practices with:
- Proper semantic HTML structure
- Consistent styling classes
- Clean PHP integration
- Well-organized sections for features, reviews and guarantees
css/frm_admin.css (1)
9489-9492: LGTM! Clean and consistent CTA icon styling.
The padding and border styling follows best practices.
|
@tuguirazvan I tried making the list wider, the gap and the font size smaller. I think the list is too small: 
|
|
@tuguirazvan I fixed other things. What is the background color of the Views page https://qa.formidableforms.com/new-submit-and-rootline/wp-admin/admin.php?page=formidable-views? Dalton told me it's |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
css/admin/dashboard.css (2)
310-311: LGTM! Consider adding fallback for older browsers.The flex layout with gap property is a good choice for spacing. However, for better browser compatibility, consider adding a fallback for browsers that don't support gap.
.frm-pro-features-list { display: flex; gap: var(--gap-2xl); + /* Fallback for browsers that don't support gap */ + & > * + * { + margin-left: var(--gap-2xl); + } }
348-391: Consider responsive behavior of the guarantee icon.The absolute positioning of the guarantee icon might cause layout issues on smaller screens, especially with the negative right value.
Consider adjusting the guarantee icon positioning for better responsiveness:
.frm-testimonial__guarantee-icon img { position: absolute; bottom: -15px; - right: -27px; + right: 0; height: 110px; width: 110px; + /* Add media query for larger screens */ + @media (min-width: 768px) { + right: -27px; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
classes/views/frm-settings/settings_cta.php(1 hunks)classes/views/shared/admin-cta.php(2 hunks)css/admin/dashboard.css(3 hunks)js/admin/applications.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/views/frm-settings/settings_cta.php
- classes/views/shared/admin-cta.php
- js/admin/applications.js
The background is white, but I have added a darker gray just under the cards at top which I guess is more complex to achieve. For now to simplify things, please, use the same background color we have for Applications tab/page. @truongwp I have a few more things for you below.
|
|
@tuguirazvan I updated it. Please check again. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
classes/views/shared/views-info.php (2)
16-18: Consider moving inline styles to stylesheet.The inline styles for centering and spacing could be moved to a dedicated CSS class in the stylesheet for better maintainability.
-<div class="frmcenter frm-m-12" style="max-width:706px;margin:auto;padding-top:50px;"> +<div class="frmcenter frm-m-12 frm-views-content">Add to stylesheet:
.frm-views-content { max-width: 706px; margin: auto; padding-top: 50px; }
246-252: Modernize float clearing method.Replace the float clearing hack with modern CSS.
-.frm-views-guarantee:after { - content: ''; - display: block; - height: 0; - visibility: hidden; - clear: both; -} +.frm-views-guarantee { + display: flow-root; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/controllers/FrmAppController.php(2 hunks)classes/views/shared/views-info.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/controllers/FrmAppController.php
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
classes/controllers/FrmAddonsController.php (3)
Line range hint
1414-1431: Consider enhancing URL validation and documentation.While the current security measures are good, consider these improvements:
- Add URL scheme validation to ensure only HTTPS URLs are allowed
- Document security implications in the
frm_allowed_external_urlsfilter descriptionApply this enhancement:
private static function allowed_external_urls() { $allowed_url_list = array( 'https://downloads.wordpress.org/plugin/formidable-gravity-forms-importer.zip', 'https://downloads.wordpress.org/plugin/formidable-import-pirate-forms.zip', 'https://downloads.wordpress.org/plugin/wp-mail-smtp.zip', ); /** * List of URLs used in plugin formidable internal installation. * * @since 6.3.1 + * @param array $allowed_url_list List of URLs. Only HTTPS URLs are supported for security. + * Adding non-HTTPS URLs will result in installation failure. */ $allowed_url_list = apply_filters( 'frm_allowed_external_urls', $allowed_url_list ); if ( ! is_array( $allowed_url_list ) ) { _doing_it_wrong( __METHOD__, 'Only an array of URLs could be used within this filter.', '6.3.1' ); return array(); } + // Ensure all URLs use HTTPS + $allowed_url_list = array_filter( $allowed_url_list, function( $url ) { + return strpos( $url, 'https://' ) === 0; + }); return $allowed_url_list; }
Line range hint
1414-1431: Enhance error handling with more descriptive messages.Consider improving error messages to provide more context and actionable information to users.
Apply these improvements:
- return array( - 'message' => 'Plugin URL is not valid', - 'success' => false, - ); + return array( + 'message' => sprintf( + 'Invalid plugin URL. The URL must be from an allowed source (%s) or the Formidable Forms S3 bucket.', + implode(', ', array_map('parse_url', self::allowed_external_urls(), PHP_URL_HOST)) + ), + 'success' => false, + );
Line range hint
1414-1431: Enhance documentation and code readability.Consider these improvements for better maintainability:
- Add more detailed PHPDoc blocks with @throws annotations
- Extract complex conditions into well-named methods
Example improvement:
+ /** + * Renders an upgrade link button for an addon + * + * @since 4.09.01 + * + * @param array|false $addon The addon data array or false if not found + * @param array|string $upgrade_link The upgrade link URL or array of attributes + * @throws InvalidArgumentException If upgrade link is invalid + * + * @return void + */ public static function addon_upgrade_link( $addon, $upgrade_link ) {classes/views/shared/views-info.php (2)
16-20: Move inline styles to stylesheet.The inline styles for centering and width constraints should be moved to a dedicated stylesheet for better maintainability.
-<div class="frmcenter frm-m-12" style="max-width:706px;margin:auto;padding-top:50px;"> +<div class="frmcenter frm-m-12 frm-views-info-content">Add to stylesheet:
.frm-views-info-content { max-width: 706px; margin: auto; padding-top: 50px; }
106-106: Remove duplicate description text.This description is identical to the one used earlier in the page. Consider using unique content that provides additional value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/controllers/FrmAddonsController.php(2 hunks)classes/views/shared/views-info.php(2 hunks)
🔇 Additional comments (5)
classes/controllers/FrmAddonsController.php (1)
1414-1414: LGTM! The parameter addition improves flexibility.
The addition of the $text parameter with a default value is a good improvement that:
- Allows customization of the button text
- Maintains backward compatibility
- Properly handles internationalization
Also applies to: 1431-1431
classes/views/shared/views-info.php (4)
164-167: LGTM! Well-structured guarantee section.
The guarantee section uses proper semantic HTML and clear content structure.
172-267: Move styles to a dedicated stylesheet.
These styles should be moved to a dedicated CSS file for better maintainability, caching, and separation of concerns.
237-242:
Increase font sizes for better readability.
The font sizes of 9px are too small for comfortable reading and may cause accessibility issues. Consider increasing them to at least 12px.
.frm-views-review__rating span {
- font-size: 9px;
+ font-size: 12px;
vertical-align: middle;
}
.frm-views-review__desc {
- font-size: 9px;
+ font-size: 12px;
}Likely invalid or redundant comment.
111-113:
Add title attribute to iframe for accessibility.
The video iframe is missing a title attribute, which is important for screen readers.
-<iframe width="843" height="200" src="https://www.youtube.com/embed/gdUt8vJ33LE" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen></iframe>
+<iframe title="Formidable Views Tutorial" width="843" height="200" src="https://www.youtube.com/embed/gdUt8vJ33LE" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen></iframe>Likely invalid or redundant comment.
Thanks @truongwp It looks good. I have a few more things to change.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
css/frm_admin.css (4)
482-492: Consider adding fallback colors for gradient backgrounds.The gradient background looks good but could benefit from some improvements:
- Add a fallback background color for browsers that don't support gradients
- Consider adding vendor prefixes for better browser compatibility
- Consider using CSS custom properties for the gradient colors to make them easier to maintain
.frm-gradient, .frm-upgrade-bar { + background-color: #388CF3; /* Fallback */ background: linear-gradient(90deg, #388CF3, #1961D5, #8C7FD8) !important; color: #fff !important; border: 0 !important; } .frm-gradient.frm-button-primary:not([disabled]):hover, .frm-gradient.frm-button-primary:not([disabled]):focus { + background-color: #2871E6; /* Fallback */ background: linear-gradient(90deg, #2871E6, #9989FB) !important; }
2308-2312: Consider using CSS custom properties for the gradient colors.The note style uses a creative gradient border approach but could be improved:
.frm_note_style2 { + --note-gradient-start: #1961D5; + --note-gradient-end: #E8ABEF; background: linear-gradient(90deg, #e9effa, #fcf6fe) padding-box, - linear-gradient(90deg, #1961D5, #E8ABEF) border-box; + linear-gradient(90deg, var(--note-gradient-start), var(--note-gradient-end)) border-box; border: 1px solid transparent; }
9499-9520: Consider adding a max-width to prevent text from becoming too wide.The upgrade banner styling looks good but could benefit from a max-width constraint for better readability on very wide screens.
#frm-upgrade-banner { gap: 0; + max-width: 1200px; + margin: 0 auto; }
9536-9536: Consider using rem units for font sizes and padding.The pro tip styling uses pixel values which don't scale with user font size preferences. Consider using rem units for better accessibility.
.frm_field_list .frm_pro_tip { - font-size: 14px; + font-size: 0.875rem; } .frm-tip-badge { - font-size: 9px; + font-size: 0.5625rem; - padding: 3px 4px; + padding: 0.1875rem 0.25rem; }Also applies to: 9553-9567
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
css/frm_admin.css(6 hunks)
🔇 Additional comments (2)
css/frm_admin.css (2)
493-507: LGTM! The upgrade bar styling follows best practices.
The styles are well-structured and use appropriate selectors. The text decoration and focus outline provide good accessibility.
9494-9497: LGTM! The CTA icon styling is clean and maintainable.
The padding and border styling use currentColor which is a good practice for maintaining consistency with the parent color.
|
@tuguirazvan I updated it. Please check the hovered button gradient as well. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
classes/views/shared/views-info.php (2)
17-31: Move inline styles to stylesheet.The inline styles for margins and max-width should be moved to the stylesheet for better maintainability.
-<div class="frmcenter frm-m-12" style="max-width:706px;margin:auto;padding-top:50px;"> -<h2 style="margin-bottom:15px;"> -<p style="max-width:400px;margin:0 auto 32px;"> +<div class="frmcenter frm-m-12 frm-views-container"> +<h2 class="frm-views-title"> +<p class="frm-views-description">Add to stylesheet:
.frm-views-container { max-width: 706px; margin: auto; padding-top: 50px; } .frm-views-title { margin-bottom: 15px; } .frm-views-description { max-width: 400px; margin: 0 auto 32px; }
164-167: Move heading styles to stylesheet.The inline styles for the heading should be moved to the stylesheet.
-<h4 style="font-weight: 600;"> +<h4 class="frm-views-guarantee__title">Add to stylesheet:
.frm-views-guarantee__title { font-weight: 600; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/views/shared/views-info.php(2 hunks)
🔇 Additional comments (2)
classes/views/shared/views-info.php (2)
Line range hint 1-16: LGTM! Well-structured container with proper security check.
The code follows security best practices and maintains consistent class naming.
238-243:
Increase font sizes for better readability.
The font sizes of 9px are too small for comfortable reading and may cause accessibility issues.
-.frm-views-review__rating span {
- font-size: 9px;
+ font-size: 12px;
}
-.frm-views-review__desc {
- font-size: 9px;
+ font-size: 12px;
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
classes/views/shared/views-info.php (4)
16-16: Move inline styles to stylesheet.Consider moving the inline styles to the stylesheet for better maintainability and separation of concerns.
-<div class="frmcenter frm-m-12" style="max-width:706px;margin:auto;padding-top:50px;"> +<div class="frmcenter frm-m-12 frm-views-container">Add to stylesheet:
.frm-views-container { max-width: 706px; margin: auto; padding-top: 50px; }
40-42: Enhance feature descriptions to highlight specific benefits.Some feature descriptions are generic or unclear about their value proposition. Consider making them more specific and benefit-focused:
- Grid: "Display entries in a responsive grid layout with customizable columns"
- Calendar: Current description is good
- Table: "Create sortable and filterable tables to organize your entries"
- Map: Current description is good
- Classic: "Build custom views with complete control over layout and styling"
- Ready made solution: Current description is good
Also applies to: 50-52, 61-63, 72-74, 83-85, 94-96
106-106: Remove duplicate description text.This description is identical to the one used earlier in the page. Consider using different text to provide additional value or remove redundancy.
166-166: Move heading style to stylesheet.Move the inline font-weight style to the stylesheet.
-<h4 style="font-weight: 600;"><?php esc_html_e( '100% No-Risk, Money Back Guarantee!', 'formidable' ); ?></h4> +<h4 class="frm-views-guarantee__title"><?php esc_html_e( '100% No-Risk, Money Back Guarantee!', 'formidable' ); ?></h4>Add to stylesheet:
.frm-views-guarantee__title { font-weight: 600; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/views/shared/views-info.php(2 hunks)
🔇 Additional comments (2)
classes/views/shared/views-info.php (2)
172-268: Move styles to a dedicated stylesheet.
The inline styles should be moved to a dedicated CSS file for better maintainability, caching, and separation of concerns.
238-243: 🛠️ Refactor suggestion
Increase font sizes for better readability.
The font sizes of 9px are too small for comfortable reading and may cause accessibility issues. Consider increasing them to at least 12px.
.frm-views-review__rating span {
- font-size: 9px;
+ font-size: 12px;
vertical-align: middle;
}
.frm-views-review__desc {
- font-size: 9px;
+ font-size: 12px;
}Likely invalid or redundant comment.
Hey Truong, The gradient button is visible only on the "Applications" page. Take a look at my design and please update the gradient button accordingly. Recording: https://cln.sh/kDGTyc41 |
|
@truongwp I have a couple of feedback items from Steve we should probably address first before I merge this.
From Steve:
Thank you! |
|
@tuguirazvan Thank you. I missed that button. Can you check again? |
|
@Crabcyborg I updated the pro tip. The whole pro tip is now a link. @tuguirazvan Do you think we should apply the hover styling of the button for the pro tip when hovering on? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
css/frm_admin.css (2)
481-491: Consider using CSS variables for gradient colors.The gradient implementation could be improved by:
- Using CSS variables for colors to maintain consistency
- Removing !important declarations if possible by increasing specificity instead
.frm-gradient, .frm-upgrade-bar { - background: linear-gradient(90deg, #388CF3, #1961D5, #8C7FD8) !important; + background: linear-gradient(90deg, var(--primary-500), var(--primary-700), var(--purple)) !important; color: #fff !important; border: 0 !important; } .frm-gradient.frm-button-primary:not([disabled]):hover, .frm-gradient.frm-button-primary:not([disabled]):focus { - background: linear-gradient(90deg, #2871E6, #9989FB) !important; + background: linear-gradient(90deg, var(--primary-700), var(--purple)) !important; }
2307-2311: LGTM! Clean implementation of gradient border.The border gradient implementation is well done using modern CSS techniques. Consider using CSS variables for the gradient colors to maintain consistency with the theme.
.frm_note_style2 { - background: linear-gradient(90deg, #e9effa, #fcf6fe) padding-box, - linear-gradient(90deg, #1961D5, #E8ABEF) border-box; + background: linear-gradient(90deg, var(--primary-25), var(--purple-25)) padding-box, + linear-gradient(90deg, var(--primary-700), var(--purple)) border-box; border: 1px solid transparent; }classes/controllers/FrmDashboardController.php (2)
Line range hint
65-116: LGTM! Consider adding PHPDoc param tags for clarity.The method effectively encapsulates dashboard data initialization, improving code organization. However, the PHPDoc block could be enhanced.
Add param tags to document the internal variables:
/** * Gets dashboard helper instance. * * @since x.x + * @param array $latest_available_form The latest available form data + * @param array $total_payments The total payments data + * @param array $counters_value The counter values for forms and entries * * @return FrmDashboardHelper */
Line range hint
118-128: LGTM! Consider adding error handling.The method effectively separates rendering from data preparation. However, it might benefit from basic error handling.
Consider adding error handling for the template inclusion:
public static function route() { $dashboard_view = self::get_dashboard_helper(); $should_display_videos = is_callable( 'FrmProDashboardHelper::should_display_videos' ) ? FrmProDashboardHelper::should_display_videos() : true; - require FrmAppHelper::plugin_path() . '/classes/views/dashboard/dashboard.php'; + $template_path = FrmAppHelper::plugin_path() . '/classes/views/dashboard/dashboard.php'; + if ( ! file_exists( $template_path ) ) { + wp_die( esc_html__( 'Dashboard template not found.', 'formidable' ) ); + } + require $template_path; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmDashboardController.php(2 hunks)classes/helpers/FrmTipsHelper.php(1 hunks)classes/views/dashboard/templates/pro-features-list.php(1 hunks)classes/views/frm-settings/settings_cta.php(1 hunks)css/frm_admin.css(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/views/frm-settings/settings_cta.php
- classes/helpers/FrmTipsHelper.php
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/views/dashboard/templates/pro-features-list.php (1)
22-35: LGTM! Clean and secure implementation.The pro features list section is well-structured with proper security measures (escaping) in place.
| <div class="frm-pro-features-list-right"> | ||
| <div class="frm-testimonial-wrapper"> | ||
| <div class="frm-testimonial"> | ||
| <div class="frm-testimonial__content">Amazing plugin, amazing support. We've been using FF since 2016. The best form plugin on WP. Its powerful and versatile with an amazing support!</div> | ||
| <div class="frm-testimonial__author">Emmanuel Khoury</div> | ||
| <div class="frm-testimonial__rating"> | ||
| <?php FrmAddonsHelper::show_five_star_rating( '#FFD966' ); ?> | ||
| </div> | ||
| <div class="frm-testimonial__guarantee-icon"> | ||
| <?php FrmAddonsHelper::guarantee_icon(); ?> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the testimonial content configurable.
The testimonial section is well-structured, but the hardcoded content makes it difficult to update or translate.
Consider moving the testimonial content to a configuration array or database setting:
+<?php
+$testimonial = array(
+ 'content' => __('Amazing plugin, amazing support. We\'ve been using FF since 2016. The best form plugin on WP. Its powerful and versatile with an amazing support!', 'formidable'),
+ 'author' => __('Emmanuel Khoury', 'formidable'),
+);
+?>
<div class="frm-pro-features-list-right">
<div class="frm-testimonial-wrapper">
<div class="frm-testimonial">
- <div class="frm-testimonial__content">Amazing plugin, amazing support. We've been using FF since 2016. The best form plugin on WP. Its powerful and versatile with an amazing support!</div>
- <div class="frm-testimonial__author">Emmanuel Khoury</div>
+ <div class="frm-testimonial__content"><?php echo esc_html( $testimonial['content'] ); ?></div>
+ <div class="frm-testimonial__author"><?php echo esc_html( $testimonial['author'] ); ?></div>This change would:
- Make the content translatable
- Make it easier to update or rotate testimonials
- Maintain proper escaping for security
📝 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.
| <div class="frm-pro-features-list-right"> | |
| <div class="frm-testimonial-wrapper"> | |
| <div class="frm-testimonial"> | |
| <div class="frm-testimonial__content">Amazing plugin, amazing support. We've been using FF since 2016. The best form plugin on WP. Its powerful and versatile with an amazing support!</div> | |
| <div class="frm-testimonial__author">Emmanuel Khoury</div> | |
| <div class="frm-testimonial__rating"> | |
| <?php FrmAddonsHelper::show_five_star_rating( '#FFD966' ); ?> | |
| </div> | |
| <div class="frm-testimonial__guarantee-icon"> | |
| <?php FrmAddonsHelper::guarantee_icon(); ?> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| <?php | |
| $testimonial = array( | |
| 'content' => __('Amazing plugin, amazing support. We\'ve been using FF since 2016. The best form plugin on WP. Its powerful and versatile with an amazing support!', 'formidable'), | |
| 'author' => __('Emmanuel Khoury', 'formidable'), | |
| ); | |
| ?> | |
| <div class="frm-pro-features-list-right"> | |
| <div class="frm-testimonial-wrapper"> | |
| <div class="frm-testimonial"> | |
| <div class="frm-testimonial__content"><?php echo esc_html( $testimonial['content'] ); ?></div> | |
| <div class="frm-testimonial__author"><?php echo esc_html( $testimonial['author'] ); ?></div> | |
| <div class="frm-testimonial__rating"> | |
| <?php FrmAddonsHelper::show_five_star_rating( '#FFD966' ); ?> | |
| </div> | |
| <div class="frm-testimonial__guarantee-icon"> | |
| <?php FrmAddonsHelper::guarantee_icon(); ?> | |
| </div> | |
| </div> | |
| </div> | |
| </div> |
Linear overview link: https://linear.app/strategy11/project/lite-to-pro-improvements-a8d5dc4f9f43/overview
QA site: https://qa.formidableforms.com/new-submit-and-rootline/wp-admin/admin.php?page=formidable-dashboard