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

Implement new simple text filter #1526

Merged
merged 16 commits into from
Jan 22, 2025
Merged

Implement new simple text filter #1526

merged 16 commits into from
Jan 22, 2025

Conversation

eddiesholl
Copy link
Contributor

@eddiesholl eddiesholl commented Jan 17, 2025

Adding a new filter type for the admin-ui-components filter bar. This is a simple text input, it has no auto complete, it just fetches with the value you have typed in. You can get an exact match if your filter value is correct. This is to help with usability issues on large list views.

There is a relatively invasive UI change here. The entity-list currently replaces the content area with a big loading image while a new filter is being applied to the query. This mens that your typing location and focus is being thrown away. It's fairly unnecessary, as the table area has its own spinner, so I'm just removing the use of the big loader image.

There is technically a breaking change in here, as the meaning of AdminUIFilterType.TEXT has changed with no migration path

I've noticed that there are duplicate enum definitions for AdminUIFilterType in the core and admin-ui-components packages. There's no dependency between these packages either way right now. I assume the solution will be to create a shared package for this type. I have no idea how many other types might be in this duplicated state

@@ -0,0 +1,57 @@
import { useLazyQuery } from '@apollo/client';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Straight clone of the previous TextFilter file

@@ -30,6 +30,11 @@ export const Input = ({
const [value, setValue] = useState<string | undefined>(initialValue ?? '');
const debouncedValue = useDebounce<string | undefined>(value, 800); // debounce request for 800ms

useEffect(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this change, the new TextFilter does not empty its value when clicking on 'Empty FIlters'. Another solution would be we have an uncontrolled Input component that does not track state like this

Copy link
Member

Choose a reason for hiding this comment

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

I think it should still be a controlled input, just not have its own state, e.g. use the filter to hold the state the way the rest of the filter components do.

@eddiesholl eddiesholl marked this pull request as ready for review January 17, 2025 05:50
Copy link
Member

@thekevinbrown thekevinbrown left a comment

Choose a reason for hiding this comment

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

Two things:

  • We should make the filter component stateless the way it is for other filters, e.g. it just stores its state in the filter itself.
  • We should make the default TEXT not DROP_DOWN_TEXT for ID and String types.

@@ -30,6 +30,11 @@ export const Input = ({
const [value, setValue] = useState<string | undefined>(initialValue ?? '');
const debouncedValue = useDebounce<string | undefined>(value, 800); // debounce request for 800ms

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

I think it should still be a controlled input, just not have its own state, e.g. use the filter to hold the state the way the rest of the filter components do.

case 'Number':
return AdminUIFilterType.NUMERIC;
case 'String':
return AdminUIFilterType.TEXT;
return AdminUIFilterType.DROP_DOWN_TEXT;
Copy link
Member

Choose a reason for hiding this comment

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

The default should be TEXT for both ID and String I reckon.

@eddiesholl eddiesholl merged commit 34ebfbb into main Jan 22, 2025
14 checks passed
@eddiesholl eddiesholl deleted the simple-text-filter branch January 22, 2025 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants