Skip to content
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

chore(hits): update banner link type #6200

Merged
merged 2 commits into from
May 21, 2024

Conversation

taylorcjohnson
Copy link
Contributor

@taylorcjohnson taylorcjohnson commented May 16, 2024

Summary

This minor PR updates makes the url property of the new banner type (part of SearchResults.renderingContent.widgets.banners) required. link is still optional, but if link is present then we want to ensure that the url is also present.

Part of EMERCH-1370

Result

Green CI (all tests still pass)

@taylorcjohnson taylorcjohnson self-assigned this May 16, 2024
Copy link

codesandbox-ci bot commented May 16, 2024

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 0b9cb7d:

Sandbox Source
example-instantsearch-getting-started Configuration
example-react-instantsearch-getting-started Configuration
example-react-instantsearch-next-app-dir-example Configuration
example-react-instantsearch-next-routing-example Configuration
example-vue-instantsearch-getting-started Configuration

@taylorcjohnson taylorcjohnson requested review from a team, Haroenv and aymeric-giraudet and removed request for a team May 16, 2024 04:24
@taylorcjohnson taylorcjohnson marked this pull request as ready for review May 16, 2024 07:04
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

looks good, no change in the code?

@taylorcjohnson
Copy link
Contributor Author

looks good, no change in the code?

Thanks for taking a look @Haroenv - that prompted me to double check and I did find one other change.

Should the Banner type be added to the SearchResults namespace within algoliasearchHelper (like Facet) and then used in SearchResults class definition or is that excessive? I assume it could be use in instantsearch-ui-components/src/components/Hits.tsx instead of manually creating a banner type there.

@Haroenv
Copy link
Contributor

Haroenv commented May 16, 2024

if I remember it correctly ui-components has no dependency on the helper, so that would maybe complicate things. I think it's good as-is

@aymeric-giraudet aymeric-giraudet merged commit 27e6f50 into master May 21, 2024
12 checks passed
@aymeric-giraudet aymeric-giraudet deleted the chore/update-banner-link-type branch May 21, 2024 07:36
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.

None yet

3 participants