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

Add Search to Hassio add-on store #3108

Merged
merged 20 commits into from May 12, 2019
Merged

Add Search to Hassio add-on store #3108

merged 20 commits into from May 12, 2019

Conversation

timmo001
Copy link
Contributor

@timmo001 timmo001 commented Apr 21, 2019

Description

Adds search functionality for add-ons in the hass.io panel:

search

This adds fuse.js as a library to the repo which is really nice for doing 'fuzzy' searches. We could use this in the future to add search to other aspects of the frontend such as adding a new card in lovelace

Copy link
Member

@bramkragten bramkragten left a comment

Choose a reason for hiding this comment

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

Do we want this to be a fuzzy search? Isn't a filter on the title of the addon enough?

const addons = this.fuzzySearchAndSort(this.addons, this.filter);

if (this.filter && addons.length < 1) {
return html``;
Copy link
Member

Choose a reason for hiding this comment

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

Should we print a message that no results are found instead of an empty screen?

Copy link
Contributor Author

@timmo001 timmo001 May 10, 2019

Choose a reason for hiding this comment

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

Now shows 'no results' message for each repository with no results:

image

hassio/src/components/hassio-search-input.ts Outdated Show resolved Hide resolved
@@ -91,6 +95,23 @@ class HassioAddonRepositoryEl extends LitElement {
navigate(this, `/hassio/addon/${ev.currentTarget.addon.slug}`);
}

private fuzzySearchAndSort(addons, filter) {
Copy link
Member

Choose a reason for hiding this comment

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

This function is in the code twice, can we reuse it instead of copy/paste?

Copy link
Member

Choose a reason for hiding this comment

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

This is called from the render method, we also re-render when hass changes. We should wrap this in memoize-one, so that a hass change won't re-calculate it all.

Suggested change
private fuzzySearchAndSort(addons, filter) {
private fuzzySearchAndSort = memoizeOne((addons, filter) => {

Copy link
Member

Choose a reason for hiding this comment

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

If you extract the method into something like filterAddons like Bram suggests, this will become memoizeOne(filterAddons)

hassio/src/dashboard/hassio-addons.js Outdated Show resolved Hide resolved
hassio/src/components/hassio-search-input.ts Outdated Show resolved Hide resolved
hassio/src/components/hassio-search-input.ts Outdated Show resolved Hide resolved
@tjorim
Copy link

tjorim commented Apr 23, 2019

Do we want this to be a fuzzy search? Isn't a filter on the title of the addon enough?

Title search only is definitely not enough. E.g. users search for a MQTT broker and type in 'MQTT',
however the title ('Mosquitto broker') does not contain that word (the description does).

@bramkragten
Copy link
Member

Title search only is definitely not enough. E.g. users search for a MQTT broker and type in 'MQTT',
however the title ('Mosquitto broker') does not contain that word (the description does).

OK, then filter on title and description... But fuzzy would also match MTQT and other user input errors. Nice, but also expensive...

@pvizeli
Copy link
Member

pvizeli commented Apr 23, 2019

I like that search handling if we can live with the resource

@timmo001
Copy link
Contributor Author

Title search only is definitely not enough. E.g. users search for a MQTT broker and type in 'MQTT',
however the title ('Mosquitto broker') does not contain that word (the description does).

OK, then filter on title and description... But fuzzy would also match MTQT and other user input errors. Nice, but also expensive...

It's very lightweight imo. The options could also be tweaked if we want, so the search can be more exact or allow other properties etc.

@timmo001
Copy link
Contributor Author

timmo001 commented Apr 24, 2019

Made a couple of changes. (Still WIP) I'll do a proper test when I get to a working hassio dev setup

@timmo001 timmo001 changed the title Add Search to Hassio add-ons WIP: Add Search to Hassio add-ons Apr 24, 2019

protected render(): TemplateResult | void {
const repo = this.repo;
const addons = memoizeOne(filterAndSort(this.addons, this.filter));
Copy link
Member

Choose a reason for hiding this comment

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

By calling memoizeOne inside render, you are creating a new instance, and so nothing is memoized. You need to store the memoized function as an instance variable like this: https://github.com/home-assistant/home-assistant-polymer/blob/dev/src/layouts/hass-router-page.ts#L55-L66

@timmo001
Copy link
Contributor Author

I think that's it. Memoized, cleaned up, css fixed, made function reusable etc.

Let me know if I missed anything 👍

search

@timmo001 timmo001 changed the title WIP: Add Search to Hassio add-ons Add Search to Hassio add-ons May 11, 2019
@timmo001 timmo001 changed the title Add Search to Hassio add-ons Add Search to Hassio add-ons store May 11, 2019
@timmo001 timmo001 changed the title Add Search to Hassio add-ons store Add Search to Hassio add-on store May 11, 2019
@pvizeli pvizeli merged commit a89f0bd into home-assistant:dev May 12, 2019
@timmo001 timmo001 deleted the hassio-search branch May 12, 2019 21:16
@balloob balloob mentioned this pull request May 14, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants