[CDX-277] Add missing Data-Attributes

[Alexey-Pavlov]

No description provided.

[Copilot]

Pull Request Overview

This PR adds missing Constructor.io data attributes to improve tracking capabilities across product cards and PLP containers. The changes ensure proper attribution for sponsored listings, search terms, zero-result states, and conversion actions.

• Conditionally adds data attributes for price, variation ID, and sponsored listing information to product cards
• Adds search term, zero-result, and section attributes to PLP containers with proper loading state handling
• Introduces a new helper function for conversion button tracking attributes

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/utils/dataAttributeHelpers.ts	Enhanced data attribute helpers to support optional item parameter, conditional attributes, and added new conversion button helper function
src/hooks/useCioPlp.ts	Extracted status variable and passed loading state to PLP container attributes function
src/components/ProductCard/ProductCard.tsx	Updated to pass item to data attributes function and added conversion tracking to add-to-cart button
spec/utils.test.tsx	Added comprehensive test coverage for new data attribute functions and edge cases
spec/components/ProductCard/productCard.test.js	Added tests to verify data attributes are properly rendered on product cards
spec/components/CioPlpGrid/CioPlpGrid.test.jsx	Updated import statement and added assertion for zero-result attribute

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[evanyan13]

Hi @Alexey-Pavlov
Thank you for working on the changes!
Left quite a few comments for your consideration please 😄

[TarekAlQaddy]

Thanks @Alexey-Pavlov and @evanyan13!
LGTM! Left a minor comment but shouldn't be a blocker

[TarekAlQaddy]

Is there a reason why we are not checking on the exact value here?

[TarekAlQaddy]

could you add additional test to check that the price / variation data attributes don't exist when they are not provided as they are conditional

[Alexey-Pavlov]

Done, thank you!

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

The PR systematically refactors data attributes into a centralized configuration object (cnstrcDataAttrs), improving maintainability and reducing hardcoded strings across the codebase.

🚨 Critical Issues

None identified.

⚠️ Important Issues

[File: src/utils/dataAttributeHelpers.ts Lines 95-96]
Non-null assertion operators (!) are used for filterName and filterValue without proper type narrowing:

[cnstrcDataAttrs.browse.filterName]: filterName\!,
[cnstrcDataAttrs.browse.filterValue]: filterValue\!,

The function should validate these values exist before accessing them, or the type signature should indicate they are required. Consider:

if (\!filterName || \!filterValue) {
  return {};
}

[File: spec/utils/dataAttributeHelpers.test.tsx Lines 46-48]
Test uses empty labels object but doesn't explicitly test the behavior when labels is undefined:

const dataAttributes = getProductCardCnstrcDataAttributes(mockProductInfo, {
  labels: {
    sl_campaign_id: 'campaign-123',
    sl_campaign_owner: 'owner-456',
  },
});

Add a test case to verify the function handles missing labels gracefully (it currently does, but this should be explicitly tested).

[File: spec/components/ProductCard/productCard.test.js Lines 418-492]
Three new test cases are excellent additions, but they test implementation details rather than user-facing behavior. Tests directly check data attributes which are internal implementation details. Consider verifying the tracking behavior instead, or ensure these attributes are part of the public API contract.

💡 Suggestions

[File: src/utils/dataAttributeHelpers.ts Lines 19-42]
The cnstrcDataAttrs object mixes common, search, and browse attributes. Consider documenting which attributes are required vs optional, and under what circumstances each is used. A JSDoc comment would improve developer experience:

/**
 * Centralized data attribute names for Constructor.io tracking
 * @example
 * // Use for product cards
 * element.setAttribute(cnstrcDataAttrs.common.itemId, 'product-123');
 */
export const cnstrcDataAttrs = {

[File: src/utils/dataAttributeHelpers.ts Lines 61-62]
Explicit null check is good, but could be simplified:

if (itemPrice \!== undefined && itemPrice \!== null) {

Could be:

if (itemPrice \!= null) {  // Checks both null and undefined

[File: spec/utils/dataAttributeHelpers.test.tsx Lines 15-16]
Import organization could be improved - group imports by type (utilities, hooks, test utilities, mock data):

// Utils
import { transformResultItem, ... } from '../../src/utils';
// Hooks
import useProductInfo from '../../src/hooks/useProduct';
// Test utilities
import { renderHookWithCioPlp } from '../test-utils';
// Mock data
import mockItem from '../local_examples/item.json';

[File: spec/utils/transformers.test.ts Line 1]
File moved from spec/transformers.test.ts to spec/utils/transformers.test.ts. Consider also moving other top-level test files to match the source structure for consistency.

[File: spec/components/ProductSwatch/ProductSwatch.test.js Lines 97-98]
The test assertion switched from checking cnstrcVariationId to cnstrcItemVariationId. Ensure this naming change is intentional and documented, as it may affect consumers of this API.

[File: spec/utils/dataAttributeHelpers.test.tsx Lines 122-133]
Good test coverage for browse data attributes, but consider adding a test for when filterName/filterValue are missing to ensure graceful handling.

[General]
The PR removes spec/utils.test.tsx and creates a new spec/utils/dataAttributeHelpers.test.tsx. This is good organization, but ensure the original test coverage is preserved. The new file appears to expand test coverage significantly, which is excellent.

[General]
Consider adding TypeScript const assertions to the cnstrcDataAttrs object to provide better type inference:

export const cnstrcDataAttrs = {
  common: { ... },
  search: { ... },
  browse: { ... },
} as const;

Overall Assessment: ⚠️ Needs Work

The PR provides excellent refactoring with strong test coverage, but the non-null assertion operators in browse attributes need to be addressed before merge to prevent potential runtime errors.