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

feat: [M3-7016] - Add Basic Filtering for Firewall Devices #9626

Merged
merged 18 commits into from
Sep 12, 2023
Merged

feat: [M3-7016] - Add Basic Filtering for Firewall Devices #9626

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
66dd46a
initial firewall device filter commit
TylerWJ Sep 1, 2023
dce81a9
Added changeset: Added Basic Filtering for Firewall Devices
TylerWJ Sep 1, 2023
746ff41
cleaned up label content in TextField
TylerWJ Sep 5, 2023
6e26452
initial ActionsPanel Storybook commit
TylerWJ Sep 8, 2023
29d3481
reverted ActionsPanel changes
TylerWJ Sep 8, 2023
14a41ba
updated storybook
TylerWJ Sep 8, 2023
56901f8
Fixed storybook
TylerWJ Sep 8, 2023
936f0a4
Merge branch 'feature/firewall-nodebalancer' into M3-7016-Add-Basic-F…
hana-akamai Sep 11, 2023
8511c04
Merge branch 'feature/firewall-nodebalancer' into M3-7016-Add-Basic-F…
hana-akamai Sep 11, 2023
66201d7
test commit - eslint issues
TylerWJ Sep 11, 2023
a61aab4
test commit
TylerWJ Sep 11, 2023
2e910e8
fixed text field
TylerWJ Sep 11, 2023
5a3e308
removed unnecessary code
TylerWJ Sep 11, 2023
f4f0a4a
added Alban's changes
TylerWJ Sep 12, 2023
a0ac06e
merged and fixed conflicts
TylerWJ Sep 12, 2023
224b342
test commit
TylerWJ Sep 12, 2023
6fd0ed6
removed test comment
TylerWJ Sep 12, 2023
dbfe946
fixed filtering error
TylerWJ Sep 12, 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
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-9626-upcoming-features-1693604222356.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Upcoming Features
---

Added Basic Filtering for Firewall Devices ([#9626](https://github.com/linode/manager/pull/9626))
1 change: 0 additions & 1 deletion packages/manager/src/components/DebouncedSearchTextField/DebouncedSearchTextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ interface DebouncedSearchProps extends TextFieldProps {
defaultValue?: string;
hideLabel?: boolean;
isSearching?: boolean;
label: string;
onSearch: (query: string) => void;
placeholder?: string;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/manager/src/components/SingleTextFieldForm/SingleTextFieldForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const SingleTextFieldForm = React.memo((props: Props) => {
...textFieldProps
} = props;

const _fieldName = fieldName ?? label.toLowerCase();
const _fieldName = fieldName ?? (label?.toLowerCase() || '');
const _successMessage = successMessage ?? `${label} updated successfully.`;
const _errorMessage = errorMessage ?? `Error updating ${label}.`;

Expand Down
40 changes: 21 additions & 19 deletions packages/manager/src/components/TextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import KeyboardArrowDown from '@mui/icons-material/KeyboardArrowDown';
import { Box } from 'src/components/Box';
import { Theme } from '@mui/material/styles';
import {
default as _TextField,
StandardTextFieldProps,
} from '@mui/material/TextField';
import { Theme } from '@mui/material/styles';
import { clamp } from 'ramda';
import * as React from 'react';
import { makeStyles } from 'tss-react/mui';

import { Box } from 'src/components/Box';
import { CircleProgress } from 'src/components/CircleProgress';
import { FormHelperText } from 'src/components/FormHelperText';
import { InputAdornment } from 'src/components/InputAdornment';
Expand Down Expand Up @@ -162,7 +162,7 @@ interface InputToolTipProps {

interface TextFieldPropsOverrides extends StandardTextFieldProps {
// We override this prop to make it required
label: string;
label?: string;
}

export type TextFieldProps = BaseProps &
Expand Down Expand Up @@ -303,22 +303,24 @@ export const TextField = (props: TextFieldProps) => {
})}
>
<Box display="flex">
<InputLabel
className={cx({
[classes.noTransform]: true,
[classes.wrapper]: noMarginTop ? false : true,
'visually-hidden': hideLabel,
})}
data-qa-textfield-label={label}
htmlFor={validInputId}
>
{label}
{required ? (
<span className={classes.label}> (required)</span>
) : optional ? (
<span className={classes.label}> (optional)</span>
) : null}
</InputLabel>
{label && (
<InputLabel
className={cx({
[classes.noTransform]: true,
[classes.wrapper]: noMarginTop ? false : true,
'visually-hidden': hideLabel,
})}
data-qa-textfield-label={label}
htmlFor={validInputId}
>
{label}
{required ? (
<span className={classes.label}> (required)</span>
) : optional ? (
<span className={classes.label}> (optional)</span>
) : null}
</InputLabel>
)}
{labelTooltipText && (
<TooltipIcon
sxTooltipIcon={{
Expand Down
93 changes: 57 additions & 36 deletions packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallDeviceLanding.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import Grid from '@mui/material/Unstable_Grid2';
import { useTheme } from '@mui/material/styles';
import { styled } from '@mui/material/styles';
import Grid from '@mui/material/Unstable_Grid2';
import * as React from 'react';

import { Button } from 'src/components/Button/Button';
import { DebouncedSearchTextField } from 'src/components/DebouncedSearchTextField';
import { Notice } from 'src/components/Notice/Notice';
import { Typography } from 'src/components/Typography';
import { useAllFirewallDevicesQuery } from 'src/queries/firewalls';
Expand All @@ -11,7 +13,7 @@ import { AddDeviceDrawer } from './AddDeviceDrawer';
import { FirewallDevicesTable } from './FirewallDevicesTable';
import { RemoveDeviceDialog } from './RemoveDeviceDialog';

import type { FirewallDeviceEntityType } from '@linode/api-v4';
import type { FirewallDevice, FirewallDeviceEntityType } from '@linode/api-v4';

export interface FirewallDeviceLandingProps {
disabled: boolean;
Expand All @@ -33,9 +35,15 @@ export const FirewallDeviceLanding = React.memo(
firewallID
);

const theme = useTheme();

const devices =
allDevices?.filter((device) => device.entity.type === type) || [];

const [filteredDevices, setFilteredDevices] = React.useState<
FirewallDevice[]
>(devices);

const [
isRemoveDeviceDialogOpen,
setIsRemoveDeviceDialogOpen,
Expand All @@ -55,6 +63,16 @@ export const FirewallDeviceLanding = React.memo(
setDeviceDrawerOpen(false);
};

const [searchText, setSearchText] = React.useState('');

const filter = (value: string) => {
setSearchText(value);
const filtered = devices.filter((device) => {
return device.entity.label.toLowerCase().includes(value.toLowerCase());
});
setFilteredDevices(filtered);
};

const formattedType = formattedTypes[type];

return (
Expand All @@ -68,31 +86,49 @@ export const FirewallDeviceLanding = React.memo(
variant="error"
/>
) : null}
<Grid container direction="column">
<Grid style={{ paddingBottom: 0 }}>
<StyledTypography>
The following {formattedType}s have been assigned to this
Firewall. A {formattedType} can only be assigned to a single
Firewall.
</StyledTypography>
<Grid
container
direction="column"
sx={{ marginBottom: theme.spacing(2) }}
>
<StyledTypography>
The following {formattedType}s have been assigned to this Firewall.
A {formattedType} can only be assigned to a single Firewall.
</StyledTypography>
<Grid
alignItems="center"
container
direction="row"
justifyContent="space-between"
>
<Grid sx={{ width: '30%' }}>
<DebouncedSearchTextField
onSearch={(val) => {
filter(val);
}}
expand={true}
placeholder={`Search ${formattedType}s`}
value={searchText}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

you may want to set a lower debounceTime than the default (250 instead of 400)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! is there a reason its at 400? Should we change the default to something lower?

Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to discuss this for a later PR, but see how the experience feels with 400. I find it to feel laggy

</Grid>
<Grid>
<Button
buttonType="primary"
data-testid="add-device-button"
disabled={disabled}
onClick={() => setDeviceDrawerOpen(true)}
>
Add {formattedType}s to Firewall
</Button>
Comment on lines +125 to +132
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use ActionsPanel here to avoid variations for primary actions button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems like the ActionsPanel is also dependent on the primary actions button. Let me know if I misinterpreted your comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tyler-akamai Yes, you are right. My intention is to use the ActionsPanel component to render the Primary button instead of having isolated references to the primary button, unless the ActionsPanel doesn't meet our requirements. This approach would make future maintenance easier.

Copy link
Contributor Author

@tyler-akamai tyler-akamai Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a good callout. I checked with the team on this. They said that for one off selections like "Add a Nodebalancer/Linode" we should use the Button component. We should only use the ActionsPanel component if there are multiple actions to select from, or we are in a modal or drawer.

Copy link
Contributor Author

@tyler-akamai tyler-akamai Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I added an ActionsPanel storybook component and added this clarification to the documentation page.

</Grid>
</Grid>
<StyledGrid>
<Button
buttonType="primary"
data-testid="add-device-button"
disabled={disabled}
onClick={() => setDeviceDrawerOpen(true)}
>
Add {formattedType}s to Firewall
</Button>
</StyledGrid>
</Grid>
<FirewallDevicesTable
triggerRemoveDevice={(id) => {
setSelectedDeviceId(id);
setIsRemoveDeviceDialogOpen(true);
}}
devices={devices ?? []}
devices={filteredDevices ?? []}
disabled={disabled}
error={error ?? undefined}
loading={isLoading}
Expand All @@ -114,22 +150,7 @@ export const FirewallDeviceLanding = React.memo(
const StyledTypography = styled(Typography, { label: 'StyledTypography' })(
({ theme }) => ({
fontSize: '0.875rem',
marginBottom: theme.spacing(2),
marginTop: theme.spacing(),
[theme.breakpoints.down('lg')]: {
marginLeft: theme.spacing(),
marginRight: theme.spacing(),
},
})
);

const StyledGrid = styled(Grid, { label: 'StyledGrid' })(({ theme }) => ({
'&.MuiGrid-item': {
paddingTop: 0,
},
display: 'flex',
justifyContent: 'flex-end',
marginBottom: theme.spacing(),
[theme.breakpoints.only('sm')]: {
marginRight: theme.spacing(),
},
}));