-
Notifications
You must be signed in to change notification settings - Fork 60
EPS-1573: Preview page promotion notice #1330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* @return void | ||
*/ | ||
public function rating_notice_css() { | ||
wp_enqueue_style( 'hfe-admin-style', HFE_URL . 'assets/css/admin-header-footer-elementor.css', [], HFE_VER ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: SQL Injection Risk in the preview nonce verification step.
Why: The method currently checks for a nonce on the preview request which adds a layer of security. However, if this nonce isn't properly validated or if the structure of the nonce is predictable, it could lead to vulnerabilities. Always ensure that nonce validation follows WordPress best practices to mitigate risks effectively.
How: Consider further securing nonce verification with better validation measures. Ensure the 'preview_nonce' is unique per request and hidden, and implement logging for failed nonce checks to help identify potential abuse.
* | ||
* @since 2.4.9 | ||
* @return void | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: Potential for Denial of Service if many notices are created/removed quickly due to localStorage handling and DOM manipulation.
Why: The JavaScript to manage the notice could lead to performance issues if users quickly interact with the notification multiple times, potentially resulting in heavy DOM manipulation or localStorage writes that could degrade user experience, especially on lower-power devices.
How: Consider debouncing the dismiss
function to limit how quickly it can be triggered or implemented a flag to prevent multiple dismissals in quick succession. This change would minimize potential performance impacts.
*/ | ||
public function elementor_preview_notice() { | ||
// Show notice only for page post type in preview mode | ||
if ( $this->should_show_preview_notice() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: Unsafe reliance on browser localStorage without fallbacks.
Why: If a user's browser has localStorage disabled, the promotional message will continually reappear, which could frustrate users. It's important to ensure that critical functionality remains smooth regardless of the user’s browser settings.
How: Implement a fallback mechanism that enables the temporary hiding of the notice without relying on localStorage.
font-weight: 600; | ||
margin-bottom: 2px; | ||
font-size: 15px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: Inconsistent visibility checks for notice removal leading to user confusion.
Why: If the notice visibility is not properly managed, users may experience inconsistent behavior when interacting with the notification banner, impeding its user-friendliness.
How: Refactor the script to ensure clear visibility states upon interaction. Additionally, add comments to explain each section of the script for future developers.
justify-content: center; | ||
font-size: 14px; | ||
flex-shrink: 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: CSS can potentially affect performance with many animate styles in rapid succession.
Why: Heavy animations can impact performance, especially on older devices or with multiple elements being animated simultaneously. This can lead to jank and a poor user experience.
How: Consider simplifying animations or limiting them based on user interaction. Alternatively, conditionally load animation styles only when necessary to reduce initial load time.
font-weight: 600; | ||
margin-bottom: 2px; | ||
font-size: 15px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: The use of inline styles may hinder maintainability and consistency in styling.
Why: Inline styles can lead to difficulties in managing consistent theming and maintenance as the codebase grows.
How: Extract styles into dedicated CSS files or utilize existing stylesheets, ensuring they're scoped correctly, to facilitate easier updates and increases in maintainability.
</div> | ||
|
||
<script> | ||
window.hfePromoNotice = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you minify this js and add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Description
Main Purpose: This pull request adds a promotional notice for Elementor in the footer of pages while they are in preview mode. The intent is to encourage users to consider upgrading to a premium version of Elementor by highlighting features and benefits.
Key Changes:
elementor_preview_notice()
has been introduced inclass-header-footer-elementor.php
, which displays a promotional notice at the bottom of the page when in preview mode, under specific conditions.should_show_preview_notice()
was created to determine when the notice should be displayed, ensuring it does not appear for certain post types or if the premium plugin is already activated.Additional Notes:
Screenshots
Types of changes
How has this been tested?
Checklist: