Add search to the apps overview

[bharatkashyap]

• Visual demonstration

Screen.Recording.2022-12-01.at.11.53.27.PM.mov

[bharatkashyap]

Some minor changes to the theme to make the search input look aligned with the rest of the UI

[bytasv]

I remember in some other PR it was mentioned that we should not use abbrevations and instead use full name

Suggested change

	return elements.filter((elem: any) =>
	return elements.filter((element: any) =>
	element.name.toLowerCase().includes(searchText.toLowerCase()),

[bytasv]

This pattern of returning both react component and filtered values as a pair looks pretty uncommon. Since the logic is so trivial, I'd personally consider keeping component itself inside the main component.

To separate filtering/searching logic, maybe we could keep this useSearch hook, but instead of returning component we would return only filtered results. Then the input for hook would be allApps, searchValue:

const [searchValue, setSearchValue] = useState('')
const filteredApps = useSearch(apps, searchValue)
...
<TextField key='search' value={searchValue} onChange={(event) => setSearchValue(event.target.value)}

The only reason right now why I'd keep this hook separate from main code is to anticipate possibility to use querying against API in the future maybe, but maybe even that's not needed 🤷

[Janpot]

Don't create hooks until a clear pattern of reuse emerges. The logic can go straight into toolpad/Apps/index.ts. We will extract reusable patterns once we align implementation between apps and connections

[prakhargupta1]

Q: In UX review last week, we briefly discussed showing apps in a table, in case we do that will it impact this search bar?

[Janpot]

Will it be a problem that changes made for search input will affect all inputs?

Perhaps we should start with just wrapping a TextField?

[bharatkashyap]

Agreed, updated

[Janpot]

But this is just the same but applying to all TextFields. Is there a good reason that this has to be built inside of the theme overrides? I mean, there are a lot of non-search TextFields.

[bharatkashyap]

I would say there's no urgent reason to do this, we could also only apply this styling to this particular TextField

At the same time, I also see no real drawbacks of making all text fields more dense and conserving some space, but perhaps that can wait till more comprehensive UI work.

I'll update to only apply styling to this TextField

[apedroferreira]

Looks solid!
Just left some comments about the styles, but it shouldn't be too much.

[apedroferreira]

Is there really no other way to style this text field as we want to without these a bit ugly overrides? Can't we do it with just regular props? But I understand that if the intended design is imposing certain things there might be no other way to do it.

I also noticed that the icon and the text are slightly misaligned vertically, but maybe most people wouldn't even notice:

[apedroferreira]

At least we should extract these styles to a variable above the component, so that we don't mix them with the JSX logic.

[bharatkashyap]

Is there really no other way to style this text field as we want to without these a bit ugly overrides?

In the beginning I added these overrides to the default theme so as to make all text fields look more dense and match the buttons in the theme, but in the discussion here we decided to go ahead with styling just this text field for now.

At least we should extract these styles to a variable above the component, so that we don't mix them with the JSX logic.

Agreed, I'll do this.

I also noticed that the icon and the text are slightly misaligned vertically, but maybe most people wouldn't even notice:

Agreed, this was much less noticeable in the dark theme. I'll fix this

[apedroferreira]

I think this works well, but as a short-term solution - long term we should have pagination when loading a list of apps, and the search should filter the results server-side.

[apedroferreira]

Thanks for the changes Bharat, the test looks good too!

This input is still not aligned though:

I'll approve this already anyway as that's just a small detail.