Sales API Phase 2#2318
Merged
Merged
Conversation
Contributor
## Walkthrough
A new banner display mechanism was integrated into the admin interface. The `FrmSalesApi` class now includes a `maybe_show_banner` method, which determines if a sale banner should be shown and outputs it with customizable content and styles. The `FrmAppHelper::print_admin_banner` method was updated to call this new method early, preventing other banners from displaying if the sale banner is shown. Additionally, new CSS rules were introduced to style the sale banner and its elements for consistent appearance in the admin area. JavaScript was added to handle click and dismiss interactions on the sale banner in the admin UI. An AJAX action hook was registered to handle banner dismissal requests.
## Changes
| File(s) | Change Summary |
|----------------------------------|---------------------------------------------------------------------------------------------------------------------|
| classes/models/FrmSalesApi.php | Added `maybe_show_banner` method to output a customizable sale banner; added `dismiss_banner` and `is_banner_dismissed` methods; extended `fill_sale` with banner-related fields. |
| classes/helpers/FrmAppHelper.php | Updated `print_admin_banner` to call `FrmSalesApi::maybe_show_banner` and return early if a sale banner is shown. |
| css/frm_admin.css | Added CSS rules for styling the new `#frm_sale_banner` and its child elements. |
| js/formidable_admin.js | Added event listeners to handle click and dismiss actions on the sale banner element in the admin interface. |
| classes/controllers/FrmHooksController.php | Registered new AJAX action hook `wp_ajax_frm_sale_banner_dismiss` mapped to `FrmSalesApi::dismiss_banner`. |
| resources/scss/admin/components/sales/banner.scss | Added SCSS styles for the sale banner component defining layout and appearance. |
| resources/scss/admin/frm_admin.scss | Added import statement for the new sales banner SCSS component. |
| .gitignore | Updated to ignore JavaScript source map files (`**/*.js.map`). |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant AdminUser
participant FrmAppHelper
participant FrmSalesApi
AdminUser->>FrmAppHelper: print_admin_banner()
FrmAppHelper->>FrmSalesApi: maybe_show_banner()
FrmSalesApi->>FrmSalesApi: Check for active sale with banner fields
alt Sale banner found
FrmSalesApi->>AdminUser: Output sale banner HTML
FrmSalesApi-->>FrmAppHelper: return true
FrmAppHelper-->>AdminUser: return (no further banners)
else No sale banner
FrmSalesApi-->>FrmAppHelper: return false
FrmAppHelper->>FrmAppHelper: Continue with other banner checks
endPossibly related PRs
Suggested labels
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
css/frm_admin.css (4)
10152-10159: Consider extracting banner dimensions into variables
The container uses a hard‑codedheight: 90px. For consistency with the design system, consider defining a custom property such as--sale-banner-heightand referencing it here.
10161-10165: Make the icon container width responsive
Settingwidth: 150pxmay cause overflow on narrow screens. You could switch to a relative or max width (e.g.max-width: 20%or use a breakpoint rule) to ensure the icon always stays visible.
10171-10174: Use design token for horizontal spacing
Instead ofmargin-right: 50px, prefer a gap variable for consistency (var(--gap-md)orvar(--gap-lg)).- margin-left: auto; - margin-right: 50px; + margin-left: auto; + margin-right: var(--gap-md);
10176-10192: Enhance CTA link styling for accessibility and theming
The CTA uses static colors (#fff/#000) and lacks hover/focus feedback. Switch to theme variables and add interactive states:- background-color: #fff; - color: #000; + background-color: var(--primary-500); + color: var(--primary-25); + + /* Interactive states */ + &:hover, + &:focus { + background-color: var(--primary-700); + outline: 2px solid var(--primary-700); + color: var(--primary-25); + }This will improve contrast, maintain design consistency, and provide necessary keyboard focus visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
images/sales/anniversary.svgis excluded by!**/*.svg,!**/*.svgimages/sales/back-to-school.svgis excluded by!**/*.svg,!**/*.svgimages/sales/black-friday.svgis excluded by!**/*.svg,!**/*.svgimages/sales/fall.svgis excluded by!**/*.svg,!**/*.svgimages/sales/geek-week.svgis excluded by!**/*.svg,!**/*.svgimages/sales/generic.svgis excluded by!**/*.svg,!**/*.svgimages/sales/no-brainer.svgis excluded by!**/*.svg,!**/*.svgimages/sales/summer.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (3)
classes/helpers/FrmAppHelper.php(1 hunks)classes/models/FrmSalesApi.php(2 hunks)css/frm_admin.css(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
classes/models/FrmSalesApi.php (1)
classes/helpers/FrmAppHelper.php (4)
FrmAppHelper(6-4478)admin_upgrade_link(121-162)array_to_html_params(1338-1347)plugin_url(65-68)
🔇 Additional comments (3)
classes/models/FrmSalesApi.php (2)
127-135: Well-structured field additions for banner customization.The new default fields for banners are well-organized and provide comprehensive customization options for sale banners including title, body, colors, and CTA elements.
239-331: Solid implementation of the banner display mechanism.The
maybe_show_bannermethod is well-implemented with:
- Proper singleton instance handling
- Validation of required banner fields (title and body)
- Default values for optional attributes
- Secure output with proper escaping via
FrmAppHelper::array_to_html_params- Clean HTML structure for the banner
The method effectively constructs a customizable banner UI with appropriate styling options and clickable behavior. The gradual fallback for CSS attributes (like colors and links) is a nice touch.
I would suggest adding a dismissible option in a future update as mentioned in the PR objectives ("sales banners should be made dismissible").
classes/helpers/FrmAppHelper.php (1)
1420-1422: Good integration with existing banner system.The code correctly checks if the sales banner should be displayed early in the function, preventing other banners from showing if a sales banner is displayed. This creates a clean priority system for banner display.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
classes/models/FrmSalesApi.php (2)
239-246: Update version in PHPDoc commentThe PHPDoc comment uses a placeholder version number "x.x" which should be updated with the actual version number before release.
- * @since x.x + * @since 6.x
346-347: Update version in PHPDoc commentSimilar to the earlier method, update the placeholder version number in the PHPDoc comment.
- * @since x.x + * @since 6.x
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmHooksController.php(1 hunks)classes/models/FrmSalesApi.php(2 hunks)js/formidable_admin.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/formidable_admin.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
classes/models/FrmSalesApi.php (1)
classes/helpers/FrmAppHelper.php (6)
FrmAppHelper(6-4474)admin_upgrade_link(121-162)array_to_html_params(1338-1347)plugin_url(65-68)icon_by_class(1199-1236)permission_check(2063-2071)
🔇 Additional comments (5)
classes/models/FrmSalesApi.php (4)
127-135: New banner display fields added correctlyThe new default fields for the banner display have been properly added to the
fill_salemethod, providing all necessary configuration options for customizing the banner appearance and content.
246-341: Banner display implementation looks goodThe implementation correctly:
- Checks for valid sales data and required fields
- Verifies if the user has already dismissed the banner
- Applies appropriate fallbacks for optional configuration values
- Properly escapes output with
esc_htmlandesc_attr- Uses the
array_to_html_paramshelper method for attribute generation- Returns boolean to indicate whether a banner was shown
This approach allows for flexible banner configuration with sensible defaults.
348-370: Banner dismissal AJAX handler implemented securelyThe implementation correctly:
- Verifies user permissions with
FrmAppHelper::permission_check- Validates the request with nonce checking
- Properly updates user options
- Returns appropriate JSON responses
Good security practices are followed throughout this method.
372-379: Well-implemented helper methodThis helper method correctly checks if a user has dismissed a specific sale banner, with proper validation of the dismissed sales array.
classes/controllers/FrmHooksController.php (1)
307-307: AJAX hook correctly registeredThe AJAX action hook for dismissing the sale banner has been properly registered, following the same pattern as other AJAX hooks in this file. This allows the banner dismissal functionality to work in the admin interface.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
classes/models/FrmSalesApi.php (4)
239-246: Update version placeholder in PHPDoc.The
@since x.xin the PHPDoc should be updated with the actual version number.- * @since x.x + * @since 6.18
286-343: Consider breaking down themaybe_show_bannermethod into smaller functions.This method is quite long and handles multiple responsibilities: attribute preparation, HTML generation, and conditional logic. Consider refactoring it into smaller helper methods for better maintainability.
public static function maybe_show_banner() { if ( ! isset( self::$instance ) ) { self::$instance = new FrmSalesApi(); } $sale = self::$instance->get_best_sale(); if ( ! $sale || ! is_array( $sale ) ) { return false; } if ( ! self::should_show_banner( $sale ) ) { return false; } self::render_banner( $sale ); return true; } +/** + * Check if the banner should be shown based on sale data and dismissal status + * + * @since 6.18 + * @param array $sale The sale data + * @return bool + */ +private static function should_show_banner( $sale ) { + $banner_title = ! empty( $sale['banner_title'] ) ? $sale['banner_title'] : false; + $banner_body = ! empty( $sale['banner_body'] ) ? $sale['banner_body'] : false; + + if ( false === $banner_title || false === $banner_body ) { + return false; + } + + return ! self::is_banner_dismissed( $sale['key'] ); +} + +/** + * Render the banner HTML + * + * @since 6.18 + * @param array $sale The sale data + * @return void + */ +private static function render_banner( $sale ) { + $banner_attrs = self::get_banner_attributes( $sale ); + $content_attrs = self::get_content_attributes( $sale ); + $cta_attrs = self::get_cta_attributes( $sale ); + $dismiss_attrs = self::get_dismiss_attributes( $sale ); + $banner_icon = ! empty( $sale['banner_icon'] ) ? $sale['banner_icon'] : 'generic'; + $banner_title = $sale['banner_title']; + $banner_body = $sale['banner_body']; + $banner_cta_text = ! empty( $sale['banner_cta_text'] ) ? $sale['banner_cta_text'] : sprintf( __( 'GET %s OFF NOW', 'formidable' ), $sale['discount_percent'] . '%' );
348-350: Update version placeholder in PHPDoc for dismiss_banner method.Similar to the previous method, update the
@since x.xin the PHPDoc with the actual version number.- * @since x.x + * @since 6.18
374-381: Missing PHPDoc@sincetag for is_banner_dismissed method.Add a
@sincetag to maintain consistency with other methods./** * @param string $key * @return bool + * @since 6.18 */ private static function is_banner_dismissed( $key ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSalesApi.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (4)
classes/models/FrmSalesApi.php (4)
127-135: Banner properties added to the sale defaults look good.The new banner-related fields have been properly added to the defaults array, providing a complete set of customization options for the sale banners.
246-265: Error handling looks good for banner validation.The implementation properly checks for the presence of the instance, an active sale, and required banner content before proceeding. Early returns prevent unnecessary code execution when conditions aren't met.
267-284: Default values and fallbacks are well implemented.The code provides sensible defaults for optional banner properties and generates a default CTA link using the existing upgrade link helper when none is provided.
350-372: Security checks in dismiss_banner are well implemented.The method properly checks user permissions and the AJAX nonce before processing the dismissal request, which is good security practice.
This was referenced Apr 30, 2025
This was referenced Oct 10, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes https://github.com/Strategy11/formidable-pro/issues/5569
TODO