Skip to content

fix(recommend): update trending facets query props (RECO-1184)#1434

Merged
shortcuts merged 2 commits intomasterfrom
fix/update-trending-queries
Dec 19, 2022
Merged

fix(recommend): update trending facets query props (RECO-1184)#1434
shortcuts merged 2 commits intomasterfrom
fix/update-trending-queries

Conversation

@marialungu
Copy link

Update trendingFacets query props and refactor types

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 16, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 41fe144:

Sandbox Source
javascript-client-app Configuration

readonly maxRecommendations?: number;

/**
* Used for trending model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't describe what this param is? (Same for facetName and facetValue for trending items.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ : 41fe144

export * from './RecommendationsQuery';
export * from './RelatedProductsQuery';
export * from './TrendingFacetsQuery';
export * from './TrendingQuery';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reasons to remove this export?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the file completely and separated the trending query in 2 independent ones

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but here is only a local type as it's only used (and needed) in getRecommendations, so there's no need to export it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more hinting the fact that it could be a breaking change now that it's not exported anymore. I think it's OK, but we should acknowledge it.

@shortcuts shortcuts merged commit 913443f into master Dec 19, 2022
@shortcuts shortcuts deleted the fix/update-trending-queries branch December 19, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants