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.

Sign up for GitHub

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

Jump to bottom

fix(templates): ensure consistency in using and recommending function/htm templates #5583

Merged
merged 5 commits into from
Apr 7, 2023
Merged

fix(templates): ensure consistency in using and recommending function/htm templates #5583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
633199d
fix(pagination): use real template
Haroenv Apr 6, 2023
3a173b7
fix(Template): easier to debug warning when string template is used
Haroenv Apr 6, 2023
50bb676
fix(helpers): deprecate highlight helpers
Haroenv Apr 6, 2023
8edb3e4
fix(panel): use undefined templates by default
Haroenv Apr 6, 2023
456f7b8
docs(stories): use function for all templates
Haroenv Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 30 additions & 21 deletions packages/instantsearch.js/.storybook/playgrounds/default.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,22 @@
import { HitsTemplates } from '../../src/widgets/hits/hits';
import { Playground } from '../decorators';

export const hitsItemTemplate = `
<div
class="hits-image"
style="background-image: url({{image}})"
></div>
<article>
<header>
<strong>{{#helpers.highlight}}{ "attribute": "name" }{{/helpers.highlight}}</strong>
</header>
<p>
{{#helpers.snippet}}{ "attribute": "description" }{{/helpers.snippet}}
</p>
<footer>
<p>
<strong>{{price}}$</strong>
</p>
</footer>
</article>
export const hitsItemTemplate: HitsTemplates['item'] = (
hit,
{ html, components }
) => html`
<div class="hits-image" style="background-image: url(${hit.image})"></div>
<article>
<header>
<strong>${components.Highlight({ hit, attribute: 'name' })}</strong>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we rather do this :

const hitsItemTemplate = (hit, { components: { Highlight } }) => html`
  <strong><${Highlight} hit=${hit} attribute="name" /></strong>
`;

If we call the component as a function Preact won't be aware it's actually a component and won't show up in devtools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it could be interesting, but in our documentation we only document the "function call" as it's much simpler to read and doesn't require people to know about preact.

Why doesn't it show up in the devtools though? it's still a preact component right?

Copy link
Member

@aymeric-giraudet aymeric-giraudet Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Making a function call would be okay if these were just render functions but as they start with a capitalized letter and take a single prop argument, it's implied they are components which should be instantiated with Preact's h as they could use hooks.

They won't show up in Devtools in the VDOM as Highlight or Snippet components because they won't ever get instantiated by Preact's h function.

Now I know it's not a big deal yet as these components are in fact just render functions, but in the future if we provide any component using hooks this could be a problem :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also do something like this developit/htm#167 (comment) (allowing the components to be a string), but I think both of those points are a separate discussion from this PR (as the PR just makes consistent what we already document), what do you think @aymeric-giraudet?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes this could be even more user-friendly !
Indeed it's a different discussion, but it's good we're already aware of the eventual shortcomings we may face in the future :D
I'll review the rest !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5584 it is possible, but I'm not convinced it's the way to go yet. We can do it easily if we want it.

</header>
<p>${components.Snippet({ hit, attribute: 'description' })}</p>
<footer>
<p>
<strong>${hit.price}$</strong>
</p>
</footer>
</article>
`;

const instantSearchPlayground: Playground = function instantSearchPlayground({
Expand All @@ -33,7 +32,7 @@ const instantSearchPlayground: Playground = function instantSearchPlayground({
typeof instantsearch.widgets.refinementList
>({
templates: {
header: 'Brands',
header: () => 'Brands',
},
})(instantsearch.widgets.refinementList);

Expand All @@ -49,7 +48,9 @@ const instantSearchPlayground: Playground = function instantSearchPlayground({

const priceMenu = instantsearch.widgets.panel<
typeof instantsearch.widgets.numericMenu
>({ templates: { header: 'Price' } })(instantsearch.widgets.numericMenu);
>({ templates: { header: () => 'Price' } })(
instantsearch.widgets.numericMenu
);

search.addWidgets([
priceMenu({
Expand All @@ -72,7 +73,7 @@ const instantSearchPlayground: Playground = function instantSearchPlayground({
typeof instantsearch.widgets.ratingMenu
>({
templates: {
header: 'Rating',
header: () => 'Rating',
},
})(instantsearch.widgets.ratingMenu);

Expand Down Expand Up @@ -127,6 +128,14 @@ const instantSearchPlayground: Playground = function instantSearchPlayground({
instantsearch.widgets.pagination({
container: pagination,
}),
instantsearch.widgets.hitsPerPage({
container: rightPanel.appendChild(document.createElement('div')),
items: [
{ label: '16 per page', value: 16 },
{ label: '32 per page', value: 32 },
{ label: '64 per page', value: 64, default: true },
],
}),
]);

const insights = instantsearch.middlewares.createInsightsMiddleware({
Expand Down
24 changes: 13 additions & 11 deletions packages/instantsearch.js/.storybook/playgrounds/movies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const demoQueryRulesPlayground: Playground = function demoQueryRulesPlayground({
typeof instantsearch.widgets.refinementList
>({
templates: {
header: 'Genres',
header: () => 'Genres',
},
})(instantsearch.widgets.refinementList);

Expand Down Expand Up @@ -53,16 +53,18 @@ const demoQueryRulesPlayground: Playground = function demoQueryRulesPlayground({
instantsearch.widgets.hits({
container: hits,
templates: {
item: `
<div
class="hits-image"
style="background-image: url({{image}})"
></div>
<article>
<header>
<strong>{{#helpers.highlight}}{ "attribute": "title" }{{/helpers.highlight}}</strong>
</header>
</article>
item: (hit, { html, components }) => html`
<div
class="hits-image"
style="background-image: url(${hit.image})"
></div>
<article>
<header>
<strong
>${components.Highlight({ attribute: 'title', hit })}</strong
>
</header>
</article>
`,
},
cssClasses: {
Expand Down
54 changes: 38 additions & 16 deletions packages/instantsearch.js/src/components/Pagination/Pagination.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { cx } from '@algolia/ui-components-shared';
import { h } from 'preact';

import { isSpecialClick } from '../../lib/utils';
import Template from '../Template/Template';

import type { ComponentCSSClasses } from '../../types';
import type {
Expand Down Expand Up @@ -58,7 +59,8 @@ function Pagination(props: PaginationProps) {
ariaLabel="First"
className={props.cssClasses.firstPageItem}
isDisabled={props.isFirstPage}
label={props.templates.first}
templates={props.templates}
templateKey="first"
pageNumber={0}
createURL={props.createURL}
cssClasses={props.cssClasses}
Expand All @@ -71,7 +73,8 @@ function Pagination(props: PaginationProps) {
ariaLabel="Previous"
className={props.cssClasses.previousPageItem}
isDisabled={props.isFirstPage}
label={props.templates.previous}
templates={props.templates}
templateKey="previous"
pageNumber={props.currentPage - 1}
createURL={props.createURL}
cssClasses={props.cssClasses}
Expand All @@ -85,7 +88,8 @@ function Pagination(props: PaginationProps) {
ariaLabel={`Page ${pageNumber + 1}`}
className={props.cssClasses.pageItem}
isSelected={pageNumber === props.currentPage}
label={`${pageNumber + 1}`}
templates={props.templates}
templateKey="page"
pageNumber={pageNumber}
createURL={props.createURL}
cssClasses={props.cssClasses}
Expand All @@ -98,7 +102,8 @@ function Pagination(props: PaginationProps) {
ariaLabel="Next"
className={props.cssClasses.nextPageItem}
isDisabled={props.isLastPage}
label={props.templates.next}
templates={props.templates}
templateKey="next"
pageNumber={props.currentPage + 1}
createURL={props.createURL}
cssClasses={props.cssClasses}
Expand All @@ -111,7 +116,8 @@ function Pagination(props: PaginationProps) {
ariaLabel="Last"
className={props.cssClasses.lastPageItem}
isDisabled={props.isLastPage}
label={props.templates.last}
templates={props.templates}
templateKey="last"
pageNumber={props.nbPages - 1}
createURL={props.createURL}
cssClasses={props.cssClasses}
Expand All @@ -124,7 +130,8 @@ function Pagination(props: PaginationProps) {
}

type PaginationLinkProps = {
label: string;
templates: PaginationTemplates;
templateKey: keyof PaginationTemplates;
ariaLabel: string;
pageNumber: number;
isDisabled?: boolean;
Expand All @@ -136,7 +143,8 @@ type PaginationLinkProps = {
};

function PaginationLink({
label,
templates,
templateKey,
ariaLabel,
pageNumber,
className,
Expand All @@ -156,17 +164,31 @@ function PaginationLink({
)}
>
{isDisabled ? (
<span
className={cssClasses.link}
dangerouslySetInnerHTML={{ __html: label }}
<Template
rootTagName="span"
rootProps={{
className: cssClasses.link,
}}
templateKey={templateKey}
templates={templates}
data={{
page: pageNumber + 1,
}}
/>
) : (
<a
className={cssClasses.link}
aria-label={ariaLabel}
href={createURL(pageNumber)}
onClick={createClickHandler(pageNumber)}
dangerouslySetInnerHTML={{ __html: label }}
<Template
rootTagName="a"
rootProps={{
className: cssClasses.link,
'aria-label': ariaLabel,
href: createURL(pageNumber),
onClick: createClickHandler(pageNumber),
}}
templateKey={templateKey}
templates={templates}
data={{
page: pageNumber + 1,
}}
/>
)}
</li>
Expand Down
8 changes: 7 additions & 1 deletion packages/instantsearch.js/src/components/Pagination/__tests__/Pagination-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,13 @@ describe('Pagination', () => {
link: 'link',
},
createURL: (args) => JSON.stringify(args),
templates: { first: '', last: '', next: '', previous: '' },
templates: {
first: '',
last: '',
next: '',
page: ({ page }) => `${page}`,
previous: '',
},
currentPage: 0,
pages: pager.pages(),
isFirstPage: pager.isFirstPage(),
Expand Down
2 changes: 1 addition & 1 deletion packages/instantsearch.js/src/components/Panel/Panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export type PanelComponentCSSClasses = ComponentCSSClasses<
>;

export type PanelComponentTemplates<TWidget extends UnknownWidgetFactory> =
Required<PanelTemplates<TWidget>>;
PanelTemplates<TWidget>;

export type PanelProps<TWidget extends UnknownWidgetFactory> = {
hidden: boolean;
Expand Down
17 changes: 11 additions & 6 deletions packages/instantsearch.js/src/components/Template/Template.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,21 @@ class Template extends Component<TemplateProps> {
}

public render() {
warning(
Object.keys(this.props.templates).every(
(key) => typeof this.props.templates[key] === 'function'
),
`Hogan.js and string-based templates are deprecated and will not be supported in InstantSearch.js 5.x.
if (__DEV__) {
const nonFunctionTemplates = Object.keys(this.props.templates).filter(
(key) => typeof this.props.templates[key] !== 'function'
);
warning(
nonFunctionTemplates.length === 0,
`Hogan.js and string-based templates are deprecated and will not be supported in InstantSearch.js 5.x.

You can replace them with function-form templates and use either the provided \`html\` function or JSX templates.

String-based templates: ${nonFunctionTemplates.join(', ')}.

See: https://www.algolia.com/doc/guides/building-search-ui/upgrade-guides/js/#upgrade-templates`
);
);
}

const RootTagName = this.props.rootTagName;

Expand Down
2 changes: 2 additions & 0 deletions packages/instantsearch.js/src/components/Template/__tests__/Template-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ describe('Template', () => {

You can replace them with function-form templates and use either the provided \`html\` function or JSX templates.

String-based templates: test.

See: https://www.algolia.com/doc/guides/building-search-ui/upgrade-guides/js/#upgrade-templates`);
});

Expand Down
10 changes: 10 additions & 0 deletions packages/instantsearch.js/src/helpers/highlight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,22 @@ export type HighlightOptions = {

const suit = component('Highlight');

/**
* @deprecated use html tagged templates and the Highlight component instead
*/
export default function highlight({
attribute,
highlightedTagName = 'mark',
hit,
cssClasses = {},
}: HighlightOptions): string {
warning(
false,
`\`instantsearch.highlight\` function has been deprecated. It is still supported in 4.x releases, but not further. It is replaced by the \`Highlight\` component.

For more information, visit https://www.algolia.com/doc/guides/building-search-ui/upgrade-guides/js/?client=html+tagged+templates#upgrade-templates`
);

const highlightAttributeResult = getPropertyByPath(
hit._highlightResult,
attribute
Expand Down
10 changes: 10 additions & 0 deletions packages/instantsearch.js/src/helpers/reverseHighlight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,22 @@ export type ReverseHighlightOptions = {

const suit = component('ReverseHighlight');

/**
* @deprecated use html tagged templates and the ReverseHighlight component instead
*/
export default function reverseHighlight({
attribute,
highlightedTagName = 'mark',
hit,
cssClasses = {},
}: ReverseHighlightOptions): string {
warning(
false,
`\`instantsearch.reverseHighlight\` function has been deprecated. It is still supported in 4.x releases, but not further. It is replaced by the \`ReverseHighlight\` component.

For more information, visit https://www.algolia.com/doc/guides/building-search-ui/upgrade-guides/js/?client=html+tagged+templates#upgrade-templates`
);

const highlightAttributeResult = getPropertyByPath(
hit._highlightResult,
attribute
Expand Down
10 changes: 10 additions & 0 deletions packages/instantsearch.js/src/helpers/reverseSnippet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,22 @@ export type ReverseSnippetOptions = {

const suit = component('ReverseSnippet');

/**
* @deprecated use html tagged templates and the ReverseSnippet component instead
*/
export default function reverseSnippet({
attribute,
highlightedTagName = 'mark',
hit,
cssClasses = {},
}: ReverseSnippetOptions): string {
warning(
false,
`\`instantsearch.reverseSnippet\` function has been deprecated. It is still supported in 4.x releases, but not further. It is replaced by the \`ReverseSnippet\` component.

For more information, visit https://www.algolia.com/doc/guides/building-search-ui/upgrade-guides/js/?client=html+tagged+templates#upgrade-templates`
);

const snippetAttributeResult = getPropertyByPath(
hit._snippetResult,
attribute
Expand Down
10 changes: 10 additions & 0 deletions packages/instantsearch.js/src/helpers/snippet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,22 @@ export type SnippetOptions = {

const suit = component('Snippet');

/**
* @deprecated use html tagged templates and the Snippet component instead
*/
export default function snippet({
attribute,
highlightedTagName = 'mark',
hit,
cssClasses = {},
}: SnippetOptions): string {
warning(
false,
`\`instantsearch.snippet\` function has been deprecated. It is still supported in 4.x releases, but not further. It is replaced by the \`Snippet\` component.

For more information, visit https://www.algolia.com/doc/guides/building-search-ui/upgrade-guides/js/?client=html+tagged+templates#upgrade-templates`
);

const snippetAttributeResult = getPropertyByPath(
hit._snippetResult,
attribute
Expand Down
Loading