feat(infinite-hits): add top banner to InfiniteHits widget #6243
Conversation
|
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 816a51b:
|
taylorcjohnson
changed the title
| <div | ||
| className={cx(cssClasses.root, cssClasses.emptyRoot)} | ||
| onClick={handleInsightsClick} | ||
| > | ||
| {banner && | ||
| (templateProps.templates.banner ? ( | ||
| <Template | ||
| {...templateProps} | ||
| templateKey="banner" | ||
| rootTagName="fragment" | ||
| data={{ | ||
| banner, | ||
| className: cssClasses.bannerRoot, | ||
| }} | ||
| /> | ||
| ) : ( | ||
| <DefaultBanner banner={banner} classNames={cssClasses} /> | ||
| ))} | ||
| <Template | ||
| {...templateProps} | ||
| templateKey="empty" | ||
| rootTagName="fragment" | ||
| data={results} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
The empty template fragment is being hoisted above the banner (even though the banner is above it in the DOM here). Any suggestions for forcing the expected layout (see Hits example in image below) without adding additional DOM nodes?
There was a problem hiding this comment.
@aymeric-giraudet or @Haroenv - any thoughts on this?
There was a problem hiding this comment.
That's odd @taylorcjohnson, is it related to CSS ?
How does it look like without our CSS stylesheets ? Maybe this should just be solved within instantsearch.css
There was a problem hiding this comment.
I don't believe so, @aymeric-giraudet - I removed the css to verify, but the DOM still shows a difference.
There was a problem hiding this comment.
I don't think it's the right solution, but what happens if you put the no results template in a div or give it a root tag name?
There was a problem hiding this comment.
Then it works as expected 😢 (either wrapping the empty template component in a div or changing the rootTagName to something else (like a div).
There was a problem hiding this comment.
Ah I think this may be due to the RawHtml component which is a hack for supporting Hogan with fragments, I'll check for a fix tomorrow morning :/
There was a problem hiding this comment.
Ah ok, I was hoping I was just missing some background context somewhere. If you can take a look I'd really appreciate it otherwise I can attempt as well.
|
|
||
| export type InfiniteHitsComponentCSSClasses = | ||
| ComponentCSSClasses<InfiniteHitsCSSClasses>; | ||
| export type InfiniteHitsComponentTemplates = Required<InfiniteHitsTemplates>; | ||
| export type InfiniteHitsComponentTemplates = InfiniteHitsTemplates; |
There was a problem hiding this comment.
This reassignment probably isn't needed anymore - unless we really need to keep the Required<> wrapper.
There was a problem hiding this comment.
It's fine to keep if other widgets do this I guess
taylorcjohnson
requested review from
a team,
sarahdayan,
Haroenv and
aymeric-giraudet
and removed request for
a team
Hey @Haroenv - sorry, this is the first I am learning about the common test suite. Does that refer to everything in If so, do we need to update the tests for And are you saying that I should remove the banner related tests for both |
|
Ah sorry I didn't notice the tests weren't done in cts for the hits implementation. Ideally yes you'd probably add a new file (suite) inside the common test suite, and also for hits. the flavour specific tests can be removed, unless it's something that exists only in one of the flavours |
|
@Haroenv thanks - I'll work on adding tests here for |
|
all tests should go exclusively in CTS when supported in more than one flavour, or planned to be. Thanks! Reach out if you are stuck somewhere |
There was a problem hiding this comment.
good for me, even though the order on empty isn't right. Not sure that matters much for now, we may even consider leaving it as-is, as I assume the banner on empty won't be that popular together with an empty template.
I'm even tempted to not display the banner on empty at all, although that would be inconsistent with Hits then.
Summary
A series of PRs recently added a top banner to the
Hitswidget (visibility is controlled by rules that updaterenderingContentfor a given query). This PR implements the same feature, but for theInfiniteHitswidget.This was done by:
connectInfiniteHits(previous PR)InfiniteHitsJS componentinfinite-hitsJS widgetInfiniteHitsReact componentInfiniteHitsReact widgetsatellite.scssandalgolia.scssRelated tickets:
Result
JS Hits, default banner:
JS InfiniteHits, default banner:
JS InfiniteHits, default banner, no results:
React Hits, default banner:
React InfiniteHits, default banner:
React InfiniteHits, default banner, no results:
