-
Notifications
You must be signed in to change notification settings - Fork 3
Feat(product-labels): Implement multiple product labels feature #544
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
base: develop
Are you sure you want to change the base?
Conversation
- Removed margin settings and replaced them with position X and Y inputs. - Added options for position unit (px or %) and anchor point (top-left or top-right). - Updated CSS class handling for absolute positioning of labels. - Introduced a new setting to enable multiple labels.
- Replaced position X and Y inputs with a new anchor point option for better label positioning. - Adjusted the range limits for position inputs to allow negative values. - Ensured consistent handling of label dimensions and styles.
- Added logic to apply rotation for text shapes 5 and 6 based on the anchor point (top-right or top-left). - Inverted rotation for top-right anchor to ensure correct visual alignment. - Set default transform properties to 'none' for other text shapes.
- Changed display properties for desktop and mobile views from flex to block. - Adjusted top positioning for regular labels to zero for better alignment. - Introduced new styles for multiple labels and their positioning. - Enhanced text shape styles to ensure consistent appearance across different shapes.
…ibility - Refactored the get_labels method to streamline label matching and display logic. - Introduced support for multiple labels with individual positioning. - Updated label HTML generation to accommodate new structure and styles. - Ensured backward compatibility for single label mode while enhancing overall functionality.
- Adjusted the minimum and maximum values for horizontal and vertical position inputs from -2000 to -300 and 2000 to 300, respectively. - Ensured more practical positioning constraints for better label alignment.
- Simplified the rotation and translation calculations for text shapes 5 and 6 based on the anchor point. - Improved code readability by using conditional variables for rotation and translation values. - Ensured consistent application of styles for both top-right and top-left anchor points.
…ed layout flexibility
…view - Added 'change' event to the document listener for improved preview updates. - Simplified conditional check for text shapes 5 and 6 rotation logic. - Set default transform properties to 'none' for better style management.
- Removed the position Y unit selection and standardized the vertical position to use 'px' for consistency. - Updated related CSS styles to ensure proper rendering of label positions.
|
|
||
| foreach ( $matching_labels as $label_data ) { | ||
| $label = $label_data['label']; | ||
| $label_html = $label_data['html']; | ||
|
|
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.
Bug: Device visibility for multiple product labels is broken, only applying settings from the first label to all.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
When multiple product labels are active, the device visibility settings are incorrectly applied. The code at inc/modules/product-labels/class-product-labels.php lines 852-869 only captures show_devices from the first matching label, due to the if ( empty( $device_visibility_classes ) ) condition. This means subsequent labels' individual device visibility configurations are ignored. As the CSS applies device visibility classes to the main container (.merchant-product-labels), all labels within that container will show or hide together based solely on the first label's settings, breaking the expected per-label device visibility functionality.
💡 Suggested Fix
Modify inc/modules/product-labels/class-product-labels.php to apply device visibility classes to each individual label wrapper, rather than only to the main container based on the first label's settings.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: inc/modules/product-labels/class-product-labels.php#L853-L857
Potential issue: When multiple product labels are active, the device visibility settings
are incorrectly applied. The code at
`inc/modules/product-labels/class-product-labels.php` lines 852-869 only captures
`show_devices` from the first matching label, due to the `if ( empty(
$device_visibility_classes ) )` condition. This means subsequent labels' individual
device visibility configurations are ignored. As the CSS applies device visibility
classes to the main container (`.merchant-product-labels`), all labels within that
container will show or hide together based solely on the first label's settings,
breaking the expected per-label device visibility functionality.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2778672
This pull request introduces the ability to display multiple product labels on a single product and significantly refactors the label positioning system for greater flexibility and control. See #519
Key Changes:
Multiple Labels Support:
Use multiple labelsswitcher has been added to the product labels settings.Refactored Positioning System:
margin_xandmargin_ysettings with a more versatileposition_x(Horizontal Position),position_y(Vertical Position), andposition_anchor(Anchor Point) system.assets/js/src/modules/product-labels/admin/preview.js) has been updated to reflect these new positioning options dynamically.Improved Text Shape Rotation:
transform-originfortext-shape-5andtext-shape-6based on theirposition_anchor. This ensures these shapes are correctly oriented regardless of whether they are anchored to the top-left or top-right.Codebase Modernization:
inc/modules/product-labels/class-product-labels.phphas been updated to support the new multiple label and flexible positioning features.assets/sass/modules/product-labels/product-labels.scsshas been adjusted to accommodate the new positioning classes and shape transformations.Testing Steps:
Access Product Labels Admin Page:
Test Single Label (Backward Compatibility & New Positioning):
Label Typeto 'Text',Text Shapeto 'Shape 1'.Anchor Pointto 'Top Left'.Horizontal Position (X)to10pxandVertical Position (Y)to10px.Anchor Pointto 'Top Right'.Horizontal Position (X)to20pxandVertical Position (Y)to20px.Label Typeto 'Text',Text Shapeto 'Shape 5'.Anchor Pointto 'Top Left',Horizontal Position (X)to0px,Vertical Position (Y)to0px.Anchor Pointto 'Top Right',Horizontal Position (X)to0px,Vertical Position (Y)to0px.Text Shape'Shape 6'. The rotation and origin should be similar to Shape 5, but with a slightly different vertical translation.Label Typeto 'Image'. Upload a custom image.Anchor Point,Horizontal Position (X), andVertical Position (Y)as in Scenarios 2.1 and 2.2.Test Multiple Labels Functionality:
Anchor Point,Horizontal Position (X), andVertical Position (Y)values.Text Shape'Shape 5' and another withText Shape'Shape 6', each with different anchor points.Test Device Visibility:
Show on devicesas 'Desktop only'.Show on devicesas 'Mobile only'.Check Console for Errors: