Feat: Add support for search highlighting #86
Conversation
mzanoni
left a comment
mzanoni
left a comment
There was a problem hiding this comment.
Really nice work! 💪
We are generating the release messages based on PR titles, this PR title is not very user friendly. Could we rename it to the features it actually brings instead?
| includeOffsets: false | ||
| }; | ||
|
|
||
| public enable(enabled: boolean): this { |
There was a problem hiding this comment.
Would like the public method to be called enabled(true) or enabled(false) as this accepts a bools, it reads better
| highlightable: this.highlightable, | ||
| limit: this.limit, | ||
| shape: this.shape | ||
| } as ContentSearchSettingsHighlightSettings; |
There was a problem hiding this comment.
Would do we need to cast here?
There was a problem hiding this comment.
No i removed it! 😊
| highlightable: this.highlightable, | ||
| limit: this.limit, | ||
| shape: this.shape | ||
| } as ProductSearchSettingsHighlightSettings; |
There was a problem hiding this comment.
Would do we need to cast here?
There was a problem hiding this comment.
No i removed it! 😊
| @@ -0,0 +1,50 @@ | |||
| import { HighlightSettings2ProductProductHighlightPropsHighlightSettings2Limits, HighlightSettings2ProductProductHighlightPropsHighlightSettings2ResponseShape, ProductHighlightProps, ProductSearchSettingsHighlightSettings } from '../../models/data-contracts'; | |||
|
|
|||
| export class ProductHighlightingBuilder { | |||
There was a problem hiding this comment.
Could we have had a base builder to unify some of the common code? Like the enabled and shape could have been part of a base builder
There was a problem hiding this comment.
The shape and limit are its own unique type for either product or content. I could not find a good way to consolidate them. 😶
SWH-Relewise
changed the title
https://trello.com/c/hfWTsKst/8562-js-sdk-update-with-new-features-from-release-11910