-
Notifications
You must be signed in to change notification settings - Fork 4
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
[DT-1081] Typescriptify the filters file #2746
Conversation
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.
looks good 👍
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.
Looks OK, but maybe datasets
doesn't need to be passed to filter items.
export const FilterItemList = (props) => { | ||
interface FilterItemListProps { | ||
category: string; | ||
datasets: DatasetTerm[] |
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.
This declares datasets
but this prop is not extracted below. Is the idea that it will be needed in the future?
max: number; | ||
minCategory: string; | ||
maxCategory: string; | ||
datasets: DatasetTerm[]; |
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.
This also appears to be unused
import { Button, TextField, Typography } from '@mui/material'; | ||
import { Checkbox } from '@mui/material'; |
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.
Not related to this PR, but I think these two imports can be merged into one
Addresses
I wanted to make this typescript before I went chasing bugs down.
Summary
Have you read Terra's Contributing Guide lately? If not, do that first.