-
-
Notifications
You must be signed in to change notification settings - Fork 274
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 the apps overview #1402
Changes from all commits
d0dc311
7542f5b
3b42cfb
7073c11
14e5d7f
07558bb
d24260d
c825358
4c90a6b
aa0c2ba
a5f303a
66523b9
05b06da
0e7cb37
08e3ba6
14fa959
96d4fa2
8b7ee62
04735d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,10 +27,12 @@ import { | |
Typography, | ||
Alert, | ||
AlertTitle, | ||
Theme, | ||
} from '@mui/material'; | ||
import LoadingButton from '@mui/lab/LoadingButton'; | ||
import ViewListIcon from '@mui/icons-material/ViewList'; | ||
import GridViewIcon from '@mui/icons-material/GridView'; | ||
import Search from '@mui/icons-material/SearchOutlined'; | ||
import invariant from 'invariant'; | ||
import { Link, useNavigate } from 'react-router-dom'; | ||
import Script from 'next/script'; | ||
|
@@ -694,6 +696,22 @@ function DemoPage() { | |
); | ||
} | ||
|
||
const searchFieldStyleOverrides = (theme: Theme) => ({ | ||
'& .MuiInputBase-root': { | ||
fontSize: theme.typography.pxToRem(14), | ||
}, | ||
'& .MuiInputBase-input': { | ||
paddingTop: theme.spacing(0.7), | ||
paddingBottom: theme.spacing(0.7), | ||
}, | ||
'& .MuiSvgIcon-root': { | ||
fontSize: theme.typography.pxToRem(16), | ||
color: theme.palette.mode === 'dark' ? theme.palette.grey[400] : theme.palette.grey[500], | ||
marginRight: theme.spacing(0.6), | ||
marginTop: theme.spacing(0.2), | ||
}, | ||
}); | ||
|
||
export default function Home() { | ||
const { | ||
data: apps = [], | ||
|
@@ -728,6 +746,18 @@ export default function Home() { | |
|
||
const AppsView = viewMode === 'list' ? AppsListView : AppsGridView; | ||
|
||
const [searchText, setSearchText] = React.useState(''); | ||
|
||
const handleSearchInput = React.useCallback((event: React.ChangeEvent<HTMLInputElement>) => { | ||
setSearchText(event.target.value); | ||
}, []); | ||
|
||
const filteredApps = React.useMemo(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return apps.filter((element: any) => | ||
element.name.toLowerCase().includes(searchText.toLowerCase()), | ||
); | ||
}, [apps, searchText]); | ||
|
||
return config.isDemo ? ( | ||
<DemoPage /> | ||
) : ( | ||
|
@@ -738,7 +768,19 @@ export default function Home() { | |
Apps | ||
</Typography> | ||
<FlexFill /> | ||
<Button onClick={() => setCreateDialogOpen(true)}>Create New</Button> | ||
<TextField | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least we should extract these styles to a variable above the component, so that we don't mix them with the JSX logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Agreed, I'll do this.
Agreed, this was much less noticeable in the dark theme. I'll fix this |
||
sx={searchFieldStyleOverrides} | ||
key={'search'} | ||
InputProps={{ | ||
startAdornment: <Search />, | ||
}} | ||
placeholder={'Search apps'} | ||
value={searchText} | ||
onChange={handleSearchInput} | ||
/> | ||
<Button variant="contained" onClick={() => setCreateDialogOpen(true)}> | ||
Create New | ||
</Button> | ||
<ToggleButtonGroup | ||
value={viewMode} | ||
exclusive | ||
|
@@ -750,23 +792,23 @@ export default function Home() { | |
aria-label="list view" | ||
color={viewMode === 'list' ? 'primary' : undefined} | ||
> | ||
<ViewListIcon /> | ||
<ViewListIcon fontSize="small" /> | ||
</ToggleButton> | ||
<ToggleButton | ||
value="grid" | ||
aria-label="grid view" | ||
color={viewMode === 'grid' ? 'primary' : undefined} | ||
> | ||
<GridViewIcon /> | ||
<GridViewIcon fontSize="small" /> | ||
</ToggleButton> | ||
</ToggleButtonGroup> | ||
</Toolbar> | ||
{error ? ( | ||
<ErrorAlert error={error} /> | ||
) : ( | ||
<Box sx={{ flex: 1, overflow: 'auto', px: 5 }}> | ||
<Box sx={{ flex: 1, overflow: 'auto', px: 5, scrollbarGutter: 'stable' }}> | ||
<AppsView | ||
apps={apps} | ||
apps={filteredApps} | ||
loading={isLoading} | ||
activeDeploymentsByApp={activeDeploymentsByApp} | ||
existingAppNames={existingAppNames} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { ToolpadHome } from '../models/ToolpadHome'; | ||
import { test, expect } from '../playwright/test'; | ||
import generateId from '../utils/generateId'; | ||
|
||
test('app search flow', async ({ page }) => { | ||
const appName1 = `App ${generateId()}`; | ||
const appName2 = `App ${generateId()}`; | ||
|
||
const homeModel = new ToolpadHome(page); | ||
await homeModel.goto(); | ||
await homeModel.createApplication({ name: appName1 }); | ||
await homeModel.goto(); | ||
await homeModel.createApplication({ name: appName2 }); | ||
await homeModel.goto(); | ||
|
||
await homeModel.searchFor(appName1); | ||
|
||
await expect(homeModel.getAppRow(appName1)).toBeVisible(); | ||
await expect(homeModel.getAppRow(appName2)).toBeHidden(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes to the theme to make the search input look aligned with the rest of the UI