-
Notifications
You must be signed in to change notification settings - Fork 54
feat: add Activity Flow data attributes support to ProductSummary com… #414
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: master
Are you sure you want to change the base?
Conversation
|
Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖 Please select which version do you want to release:
And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.
|
|
Beep boop 🤖 I noticed you didn't make any changes at the
In order to keep track, I'll create an issue if you decide now is not a good time
|
salesfelipe
left a comment
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.
Also it is missing a changelog entry
react/ProductSummaryList.tsx
Outdated
| * { 'data-af-category': 'clothing', 'data-af-position': '2' } | ||
| * ]} | ||
| */ | ||
| afDataAttributesList?: Array<Record<string, string>> |
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.
I'm wondering if it is better to be a function instead. Something like (product: Product) => Record<string,string>
So each element calls the function before rendering and gets the data attributes..
Just wondering because by receiving a fixed list, you need the data upfront, which is fine for Recommendations, but not for the other Static shelfs and since this is a shared component....
salesfelipe
left a comment
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.
It looks good, but I'm wondering wether we should be so explicity about Acitivitty Flow. This feature allow us to add data attributes to our HTML Elements, do we really want to be attached to AF on this level?
… in ProductSummary components
# Conflicts: # CHANGELOG.md # react/ProductSummaryCustom.tsx
react/ProductSummaryCustom.tsx
Outdated
| style={{ maxWidth: PRODUCT_SUMMARY_MAX_WIDTH }} | ||
| ref={inViewRef} | ||
| data-van-aid={eventParameters} | ||
| {...(extraProductProps ?? {})} |
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.
Does it make sense to put it as the first attribute on the section? The idea is to avoid overriding attributes defined above with this spread, like className, so we don't change the value of attributes set by the component itself.
| listName?: string | ||
| position?: number | ||
| placement?: string | ||
| extraProductProps?: Record<string, string> |
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.
Does it make sense to prefix all attributes with data-*? It can be a check.
Is an way to avoid others unexpected attributes.
Co-authored-by: Vinícius Seixas <[email protected]>
…yCustom Added a runtime validation to ensure all keys in extraProductProps start with 'data-'. Invalid keys will trigger a console warning. Updated the type definition for extraProductProps to enforce this requirement.
react/ProductSummaryCustom.tsx
Outdated
| // Validate that all keys in extraProductProps start with 'data-' | ||
| if (extraProductProps) { | ||
| const invalidKeys = Object.keys(extraProductProps).filter( | ||
| (key) => !key.startsWith('data-') | ||
| ) | ||
|
|
||
| if (invalidKeys.length > 0) { | ||
| console.warn( | ||
| `extraProductProps contains keys that don't start with 'data-': ${invalidKeys.join( | ||
| ', ' | ||
| )}` | ||
| ) | ||
| } | ||
| } |
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.
This code warns about keys that don't start with data-, but it doesn't filter them out, extraProductProps is still passed to the section as-is.
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.
Pull request overview
This pull request adds support for custom Activity Flow data attributes to product summary components, enabling flexible analytics and tracking capabilities.
- Introduces
extraProductPropsandbuildExtraProductPropsprops for passing custom data attributes to product summaries - Implements sanitization logic to ensure only valid
data-*attributes are applied - Extends both individual product summary and list components to support these new props
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| react/ProductSummaryCustom.tsx | Adds extraProductProps prop, implements sanitizeDataAttributes function to validate data attributes, and spreads sanitized attributes onto the section element |
| react/ProductSummaryListWithoutQuery.tsx | Introduces buildExtraProductProps function prop that generates product-specific data attributes and passes them to individual product summaries |
| react/ProductSummaryList.tsx | Adds buildExtraProductProps prop and forwards it to ProductSummaryListWithoutQuery component |
| react/utils/shouldShowSponsoredBadge.ts | Minor whitespace formatting change |
| react/mocks/vtex.render-runtime.js | Minor whitespace formatting change |
| CHANGELOG.md | Documents the addition of optional props support for Product Summary components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
vmourac-vtex
left a comment
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.
@thiagopereira-vtex pedi um review do Copilot para ajudar. Gostei de algumas recomendações, acho que vale olhar.
No mais faço só uma sugestão de mover a função de sanitização para um local mais adequado.
react/ProductSummaryCustom.tsx
Outdated
| /** | ||
| * Type for data attributes that must start with 'data-' | ||
| * Note: TypeScript 3.9 doesn't support template literal types, | ||
| * so this is a documentation type. Runtime validation ensures keys start with 'data-' | ||
| */ | ||
| type DataAttributes = Record<string, string> | ||
|
|
||
| const sanitizeDataAttributes = ( | ||
| extraProductProps?: DataAttributes | ||
| ): DataAttributes | undefined => { | ||
| if (!extraProductProps) return undefined | ||
|
|
||
| const sanitized: DataAttributes = {} | ||
| const invalidKeys: string[] = [] | ||
|
|
||
| Object.keys(extraProductProps).forEach((key) => { | ||
| if (key.startsWith('data-')) { | ||
| sanitized[key] = extraProductProps[key] | ||
| } else { | ||
| invalidKeys.push(key) | ||
| } | ||
| }) | ||
|
|
||
| if (invalidKeys.length > 0) { | ||
| console.warn( | ||
| `extraProductProps contains keys that don't start with 'data-': ${invalidKeys.join( | ||
| ', ' | ||
| )}` | ||
| ) | ||
| } | ||
|
|
||
| return sanitized |
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.
@thiagopereira-vtex Além da consideração acima feita pelo Copilot, sobre o retorno da função, sugiro somente de mover a função para fora ProductSummaryCustom, para a pasta /utils/, em um arquivo dedicado ou dentro de /utils/normalize.
A ideia é só desacoplar a função do componente para torna-la mais testável e reutilizável
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Moved the data attributes type definition and sanitization logic into a separate utility function. This improves code organization and maintains the validation of keys starting with 'data-'. Updated the component to utilize the new utility for handling extra product properties.
vmourac-vtex
left a comment
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.
LGTM
This pull request adds support for passing custom Activity Flow data attributes to product summary components, enabling more flexible analytics and tracking. The changes introduce new props to handle these attributes and ensure they are correctly spread onto relevant HTML elements in both individual product summaries and product summary lists.
Activity Flow data attributes support:
afDataAttributesprop toProductSummaryCustomandProductSummaryWrapper, allowing custom data attributes to be spread onto the section element for each product summary. [1] [2] [3] [4]afDataAttributesListprop toProductSummaryListandProductSummaryListWithoutQuery, enabling an array of data attribute objects to be passed and applied to each corresponding product summary in the list. [1] [2] [3] [4] [5] [6] [7] [8]ProductSummaryListWithoutQueryto pass the correct data attributes fromafDataAttributesListto each product summary based on its index. [1] [2]